Re: Logical Replication of sequences - Mailing list pgsql-hackers

From vignesh C
Subject Re: Logical Replication of sequences
Date
Msg-id CALDaNm1Y_ot-jFRfmtwDuwmFrgSSYHjVuy28RspSopTtwzXy8w@mail.gmail.com
Whole thread Raw
In response to Re: Logical Replication of sequences  (Peter Smith <smithpb2250@gmail.com>)
List pgsql-hackers
On Wed, 31 Jul 2024 at 12:56, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> Here are my review comments for your latest 0730_2* patches.
>
> Patch v20240730_2-0001 looks good to me.
>
> Patch v20240730_2-0002 looks good to me.
>
> My comments for the v20240730_2-0003 patch are below:
> ~~~
>
> 4. AlterSubscription_refresh
>
> My first impression (from the function comment) is that these function
> parameters are a bit awkward. For example,
> - It says:  If 'copy_data' parameter is true, the function will set
> the state to "init"; otherwise, it will set the state to "ready".
> - It also says: "If 'all_relations' is true, mark all objects with
> "init" state..."
> Those statements seem to clash. e.g. if copy_data is false but
> all_relations is true, then what (???)

all_relations will be true only for "ALTER SUBSCRIPTION ... REFRESH
PUBLICATION SEQUENCES".  With option is not supported along with this
command so copy_data with false option is not possible here. Added an
assert for this.
>
> 8.
> + *lsn = DatumGetInt64(slot_getattr(slot, 4, &isnull));
> + Assert(!isnull);
>
> Should that be DatumGetUInt64?

It should be DatumGetLSN here.

The rest of the comments are fixed. The attached v20240805 version
patch has the changes for the same.

Regards,
Vignesh

Attachment

pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: Conflict detection and logging in logical replication
Next
From: shveta malik
Date:
Subject: Re: Conflict detection and logging in logical replication