Missing comma?

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>&lt;iteration count&gt;</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>&lt;iteration
count&gt;</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

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>&lt;iteration count&gt;</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

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>&lt;iteration 
count&gt;</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

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]https://github.com/postgres/postgres/commit/7ccb2f54d9f3f3c5b4ac092d62c846b02a47f8d5 - I received it too
yesterday :-)

[1]: https://github.com/postgres/postgres/commit/7ccb2f54d9f3f3c5b4ac092d62c846b02a47f8d5
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

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

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

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

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

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