Re: Logical Replication of sequences

On Wed, 31 Jul 2024 at 12:56, Peter Smith <smithpb2250(at)gmail(dot)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