Thread: Missing comma?
Hello! https://www.postgresql.org/docs/current/catalog-pg-class.html says "Columns used to form “replica identity” for rows: d = default (primary key, if any), n = nothing, f = all columns i = index with indisreplident set, or default". Am I wrong, or is there a missing comma (or other punctuation mark) before "i = index"?.. -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
> On 13 May 2020, at 12:20, Marina Polyakova <m.polyakova@postgrespro.ru> wrote: > https://www.postgresql.org/docs/current/catalog-pg-class.html says "Columns used to form “replica identity” for rows: d= default (primary key, if any), n = nothing, f = all columns i = index with indisreplident set, or default". Am I wrong,or is there a missing comma (or other punctuation mark) before "i = index"?.. I agree with this, there should be a comma between "f = all columns" and "i = index with.." per the below: diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index ce33df9e58..cbd76e1bf5 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -1935,7 +1935,7 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l Columns used to form <quote>replica identity</quote> for rows: <literal>d</literal> = default (primary key, if any), <literal>n</literal> = nothing, - <literal>f</literal> = all columns + <literal>f</literal> = all columns, <literal>i</literal> = index with <structfield>indisreplident</structfield> set, or default </entry> </row> cheers ./daniel
Thanks!
P.S. Looking at the final sentence:
"Columns used to form “replica identity” for rows: d = default (primary
key, if any), n = nothing, f = all columns, i = index with
indisreplident set, or default"
in my opinion it's a little unclear what "or default" means at the end,
because the comma is used to separate enumeration elements ("d = default
<...>, n = nothing, f = all columns, i = index <...>") and inside the
element description ("i = index with indisreplident set, or default").
Therefore here is an additional patch from me to clarify this place,
thanks to Alexander Lakhin for help.
On 2020-05-13 16:23, Daniel Gustafsson wrote:
>> On 13 May 2020, at 12:20, Marina Polyakova
>> <m.polyakova@postgrespro.ru> wrote:
>
>> https://www.postgresql.org/docs/current/catalog-pg-class.html says
>> "Columns used to form “replica identity” for rows: d = default
>> (primary key, if any), n = nothing, f = all columns i = index with
>> indisreplident set, or default". Am I wrong, or is there a missing
>> comma (or other punctuation mark) before "i = index"?..
>
> I agree with this, there should be a comma between "f = all columns"
> and "i =
> index with.." per the below:
>
> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> index ce33df9e58..cbd76e1bf5 100644
> --- a/doc/src/sgml/catalogs.sgml
> +++ b/doc/src/sgml/catalogs.sgml
> @@ -1935,7 +1935,7 @@ SCRAM-SHA-256$<replaceable><iteration
> count></replaceable>:<replaceable>&l
> Columns used to form <quote>replica identity</quote> for rows:
> <literal>d</literal> = default (primary key, if any),
> <literal>n</literal> = nothing,
> - <literal>f</literal> = all columns
> + <literal>f</literal> = all columns,
> <literal>i</literal> = index with
> <structfield>indisreplident</structfield> set, or default
> </entry>
> </row>
>
> cheers ./daniel
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment
On Wed, May 13, 2020 at 05:17:43PM +0300, Marina Polyakova wrote:
> in my opinion it's a little unclear what "or default" means at the end,
> because the comma is used to separate enumeration elements ("d = default
> <...>, n = nothing, f = all columns, i = index <...>") and inside the
> element description ("i = index with indisreplident set, or default").
> Therefore here is an additional patch from me to clarify this place, thanks
> to Alexander Lakhin for help.
Yes, I agree that this last "or default" in the docs does not make
much sense, because we always enforce REPLICA_IDENTITY_NOTHING if
there is no replica identity.
> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> index 02ddebae99d98110a8dd290dd4cb0c980adf7984..034a08f80ea4269f131e7e1383ba482fd76d9344 100644
> --- a/doc/src/sgml/catalogs.sgml
> +++ b/doc/src/sgml/catalogs.sgml
> @@ -1936,7 +1936,7 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
> <literal>d</literal> = default (primary key, if any),
> <literal>n</literal> = nothing,
> <literal>f</literal> = all columns,
> - <literal>i</literal> = index with <structfield>indisreplident</structfield> set, or default
> + <literal>i</literal> = index with <structfield>indisreplident</structfield> set (if any)
> </entry>
> </row>
And you don't need the ("if any") either here, no?
REPLICA_IDENTITY_INDEX means that we have an index associated with the
replica identity so it seems to me that this last part can just be
removed.
--
Michael
Attachment
On 2020-05-14 03:07, Michael Paquier wrote:
> On Wed, May 13, 2020 at 05:17:43PM +0300, Marina Polyakova wrote:
>> in my opinion it's a little unclear what "or default" means at the
>> end,
>> because the comma is used to separate enumeration elements ("d =
>> default
>> <...>, n = nothing, f = all columns, i = index <...>") and inside the
>> element description ("i = index with indisreplident set, or default").
>> Therefore here is an additional patch from me to clarify this place,
>> thanks
>> to Alexander Lakhin for help.
>
> Yes, I agree that this last "or default" in the docs does not make
> much sense, because we always enforce REPLICA_IDENTITY_NOTHING if
> there is no replica identity.
This looks like a shortened version of a comment from
src/include/catalog/pg_class.h:
/*
* an explicitly chosen candidate key's columns are used as replica
identity.
* Note this will still be set if the index has been dropped; in that
case it
* has the same meaning as 'd'.
*/
#define REPLICA_IDENTITY_INDEX 'i'
>> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
>> index
>> 02ddebae99d98110a8dd290dd4cb0c980adf7984..034a08f80ea4269f131e7e1383ba482fd76d9344
>> 100644
>> --- a/doc/src/sgml/catalogs.sgml
>> +++ b/doc/src/sgml/catalogs.sgml
>> @@ -1936,7 +1936,7 @@ SCRAM-SHA-256$<replaceable><iteration
>> count></replaceable>:<replaceable>&l
>> <literal>d</literal> = default (primary key, if any),
>> <literal>n</literal> = nothing,
>> <literal>f</literal> = all columns,
>> - <literal>i</literal> = index with
>> <structfield>indisreplident</structfield> set, or default
>> + <literal>i</literal> = index with
>> <structfield>indisreplident</structfield> set (if any)
>> </entry>
>> </row>
>
> And you don't need the ("if any") either here, no?
> REPLICA_IDENTITY_INDEX means that we have an index associated with the
> replica identity so it seems to me that this last part can just be
> removed.
I tried to save information for cases when the index is dropped as in
the code comment (see above) and the function RelationGetIndexList
(where IIUC we check if this index exists):
if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex))
relation->rd_replidindex = pkeyIndex;
else if (replident == REPLICA_IDENTITY_INDEX &&
OidIsValid(candidateIndex))
relation->rd_replidindex = candidateIndex;
else
relation->rd_replidindex = InvalidOid;
Perhaps it will be useful for some users to know that relreplident = i
does not mean that the required index exists (as in default case)?..
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Thu, May 14, 2020 at 06:30:38PM +0300, Marina Polyakova wrote: > Perhaps it will be useful for some users to know that relreplident = i does > not mean that the required index exists (as in default case)?.. Yeah, that feels incomplete and I think that it would be good to close the gap here, as the user has no way to guess how things work now just by looking at the docs. What about switching your suggestion of "(if any)" to be "(same as default if the index used got dropped)"? -- Michael
Attachment
On 2020-05-15 05:16, Michael Paquier wrote: > On Thu, May 14, 2020 at 06:30:38PM +0300, Marina Polyakova wrote: >> Perhaps it will be useful for some users to know that relreplident = i >> does >> not mean that the required index exists (as in default case)?.. > > Yeah, that feels incomplete and I think that it would be good to close > the gap here, as the user has no way to guess how things work now just > by looking at the docs. What about switching your suggestion of "(if > any)" to be "(same as default if the index used got dropped)"? I like if we can explain the situation in more detail. But IMO the phrase "same as default" sounds as if we will try to find the primary index and use it if the required index (with pg_index.indisreplident = true) does not exist. What do you think of "(same as nothing if the index used got dropped)"? It seems that in this case we have the same behaviour: - we cannot update or delete rows from the table if the action is published because this table does not have a "working" replica identity; - we cannot apply updates or deletes on subscriber until we have a primary key or the published relation has replica identity full. P.S. Thank you for fixing the assertion failure [1] - I received it too yesterday :-) [1] https://github.com/postgres/postgres/commit/7ccb2f54d9f3f3c5b4ac092d62c846b02a47f8d5 -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Sat, May 16, 2020 at 09:38:46PM +0300, Marina Polyakova wrote: > I like if we can explain the situation in more detail. But IMO the phrase > "same as default" sounds as if we will try to find the primary index and use > it if the required index (with pg_index.indisreplident = true) does not > exist. What do you think of "(same as nothing if the index used got > dropped)"? It seems that in this case we have the same behaviour: > - we cannot update or delete rows from the table if the action is published > because this table does not have a "working" replica identity; > - we cannot apply updates or deletes on subscriber until we have a primary > key or the published relation has replica identity full. Yeah. I was testing that once again today and you are right. The publisher would just assume that there is nothing as there is in the changes nothing about the old row for a relation using a replident based on an index that got dropped, and this even if there is a primary key on the relation. So using "same as nothing" would be fine. (No need for logical replication to test that actually. You can just use one cluster with wal_level = logical and a slot with test_decoding to grab the same amount of information.) Would you like to send an updated patch? Note that as the release of beta1 is planned for this week, we have a grace period until the version is tagged on HEAD. -- Michael
Attachment
On 2020-05-18 09:16, Michael Paquier wrote: > On Sat, May 16, 2020 at 09:38:46PM +0300, Marina Polyakova wrote: >> I like if we can explain the situation in more detail. But IMO the >> phrase >> "same as default" sounds as if we will try to find the primary index >> and use >> it if the required index (with pg_index.indisreplident = true) does >> not >> exist. What do you think of "(same as nothing if the index used got >> dropped)"? It seems that in this case we have the same behaviour: >> - we cannot update or delete rows from the table if the action is >> published >> because this table does not have a "working" replica identity; >> - we cannot apply updates or deletes on subscriber until we have a >> primary >> key or the published relation has replica identity full. > > Yeah. I was testing that once again today and you are right. The > publisher would just assume that there is nothing as there is in the > changes nothing about the old row for a relation using a replident > based on an index that got dropped, and this even if there is a > primary key on the relation. So using "same as nothing" would be > fine. Glad to hear this =) > Would you like to send an updated patch? Note that as the release of > beta1 is planned for this week, we have a grace period until the > version is tagged on HEAD. The patch is attached, changes to html such as they were discussed. -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
On Mon, May 18, 2020 at 10:31:35PM +0300, Marina Polyakova wrote: > On 2020-05-18 09:16, Michael Paquier wrote: >> Would you like to send an updated patch? Note that as the release of >> beta1 is planned for this week, we have a grace period until the >> version is tagged on HEAD. > > The patch is attached, changes to html such as they were discussed. Looks good to me. Thanks! -- Michael
Attachment
On 2020-05-19 05:49, Michael Paquier wrote: > On Mon, May 18, 2020 at 10:31:35PM +0300, Marina Polyakova wrote: >> On 2020-05-18 09:16, Michael Paquier wrote: >>> Would you like to send an updated patch? Note that as the release of >>> beta1 is planned for this week, we have a grace period until the >>> version is tagged on HEAD. >> >> The patch is attached, changes to html such as they were discussed. > > Looks good to me. Thanks! Here is a patch with a fixed commit message (primary index => primary key). -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
On Tue, May 19, 2020 at 11:14:56AM +0300, Marina Polyakova wrote: > Here is a patch with a fixed commit message (primary index => primary key). Thanks, applied. -- Michael
Attachment
On 2020-05-20 08:26, Michael Paquier wrote: > On Tue, May 19, 2020 at 11:14:56AM +0300, Marina Polyakova wrote: >> Here is a patch with a fixed commit message (primary index => primary >> key). > > Thanks, applied. Thank you! -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company