Horton

Thread

Commits

  1. Doc: Fix description of pg_class.relreplident

  2. Fix assertion with relation using REPLICA IDENTITY FULL in subscriber

  1. Missing comma?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-05-13T10:20:41Z

    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

  2. Re: Missing comma?

    Daniel Gustafsson <daniel@yesql.se> — 2020-05-13T13:23:32Z

    > 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

  3. Re: Missing comma?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-05-13T14:17:43Z

    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

  4. Re: Missing comma?

    Michael Paquier <michael@paquier.xyz> — 2020-05-14T00:07:38Z

    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

  5. Re: Missing comma?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-05-14T15:30:38Z

    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

  6. Re: Missing comma?

    Michael Paquier <michael@paquier.xyz> — 2020-05-15T02:16:25Z

    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

  7. Re: Missing comma?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-05-16T18:38:46Z

    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

  8. Re: Missing comma?

    Michael Paquier <michael@paquier.xyz> — 2020-05-18T06:16:19Z

    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

  9. Re: Missing comma?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-05-18T19:31:35Z

    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

  10. Re: Missing comma?

    Michael Paquier <michael@paquier.xyz> — 2020-05-19T02:49:18Z

    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

  11. Re: Missing comma?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-05-19T08:14:56Z

    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

  12. Re: Missing comma?

    Michael Paquier <michael@paquier.xyz> — 2020-05-20T05:26:36Z

    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

  13. Re: Missing comma?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-05-20T09:01:20Z

    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