Re: Logical Replication of sequences

On Tue, Jun 24, 2025 at 10:37 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> >
> > Thanks for the comment, the attached v20250622 version patch has the
> > changes for the same.
> >
>
> Thanks for the patches, I am not done with review yet, but please find
> the feedback so far:
>

Thanks for the review.

>
> 1)
> + if (!OidIsValid(seq_relid))
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("sequence \"%s.%s\" does not exist",
> +    schema_name, sequence_name));
>
> ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE might not be a correct error
> code here. Shall we have ERRCODE_UNDEFINED_OBJECT? Thoughts?
>

+1
Updated to ERRCODE_UNDEFINED_OBJECT code in v20250630

>
> 2)
> tab-complete here shows correctly:
>
> postgres=# CREATE PUBLICATION pub6 FOR ALL
> SEQUENCES  TABLES
>
> But tab-complete for these 2 commands does not show anything:
>
> postgres=# CREATE PUBLICATION pub6 FOR ALL TABLES,
> postgres=# CREATE PUBLICATION pub6 FOR ALL SEQUENCES,
>
> We shall specify SEQUENCES/TABLES in above commands. IIUC, we do not
> support any other combination like (table <name>, tables in schema
> <name>) once we get ALL clause in command. So it is safe to display
> tab-complete as either TABLES or SEQUENECS in above.
>

Tab-completion is not supported after a comma (,) in any other cases.
For example, the following commands are valid, but tab-completion does
not work after the comma:

CREATE PUBLICATION pub7 FOR TABLE t1, TABLES IN SCHEMA public;
CREATE PUBLICATION pub7 FOR TABLES IN SCHEMA public, TABLES IN SCHEMA schema2;

I feel we can keep the behavior consistent in this case too. Thoughts?

> 3)
> postgres=#  CREATE publication pub1 for sequences;
> ERROR:  invalid publication object list
> LINE 1: CREATE publication pub1 for sequences;
>                                  ^
> DETAIL:  One of TABLE, TABLES IN SCHEMA or ALL SEQUENCES must be
> specified before a standalone table or schema name.
>
>
> This message is not correct as we can not have ALL SEQUENCES *before*
> a standalone table or schema name. The problem is that gram.y is
> taking *sequences* as a table or schema name. I noticed that it does
> same with *tables* as well:
>
> postgres=#  CREATE publication pub1 for tables;
> ERROR:  invalid publication object list
> LINE 1: CREATE publication pub1 for tables;
>                                   ^
> DETAIL:  One of TABLE, TABLES IN SCHEMA or ALL SEQUENCES must be
> specified before a standalone table or schema name.
>
>
> But since gram.y here can not identify an in-between missing keyword
> *all*, thus it is considering it (tables/sequenecs) as a literal/name
> instead of keyword. We can revert back to old message in such a case.
> I am unable to think of a good solution here.
>

Done, reverted to the original message.

>
> 4)
> I think the error here is wrong as we are not trying to specify
> multiple all-tables entries.
>
> postgres=# CREATE PUBLICATION pub6 for all tables, tables in schema public;
> ERROR:  invalid publication object list
> LINE 1: CREATE PUBLICATION pub6 for all tables, tables in schema pub...
>
>                                                 ^
> DETAIL:  ALL TABLES can be specified only once.
>

The parser cannot distinguish between "TABLES" and "TABLES IN SCHEMA"
while building all_object_list for "FOR ALL ...".
To address this, the duplicate check has been moved to
CreatePublication, and a syntax error is now raised for cases
mentioned above.

>
> 5)
> The log messages still has some scope of improvement:
>
> 2025-06-24 08:52:17.988 IST [110359] LOG:  logical replication
> sequence synchronization worker for subscription "sub1" has started
> 2025-06-24 08:52:18.029 IST [110359] LOG:  logical replication
> sequence synchronization for subscription "sub1" - total
> unsynchronized: 3
> 2025-06-24 08:52:18.090 IST [110359] LOG:  logical replication
> sequence synchronization for subscription "sub1" - batch #1 = 3
> attempted, 0 succeeded, 2 mismatched, 1 missing
> 2025-06-24 08:52:18.091 IST [110359] ERROR:  logical replication
> sequence synchronization failed for subscription "sub1"
> 2025-06-24 08:52:18.091 IST [110359] DETAIL:  Sequences
> ("public.myseq100") are missing on the publisher. Additionally,
> parameters differ for the remote and local sequences
> ("public.myseq101", "public.myseq102").
> 2025-06-24 08:52:18.091 IST [110359] HINT:  Use ALTER SUBSCRIPTION ...
> REFRESH PUBLICATION or use ALTER SUBSCRIPTION ... REFRESH PUBLICATION
> SEQUENCES. Alter or re-create local sequences to have the same
> parameters as the remote sequences
>
>
> a)
> Sequences ("public.myseq100") are missing on the publisher.
> Additionally, parameters differ for the remote and local sequences
> ("public.myseq101", "public.myseq102").
>
> Shall we change this to:
> missing sequence(s) on publisher: ("public.myseq100"); mismatched
> sequences(s) on subscriber: ("public.myseq101", "public.myseq102")
>
> It will then be similar to the previous log pattern ( 3 attempted, 0
> succeeded etc) instead of being more verbal. Thoughts?
>
>
> b)
> Hints are a little odd. First line of hint is just saying to use
> 'ALTER SUBSCRIPTION' without giving any purpose. While the second line
> of hint is giving the purpose of alter-sequences.
>
> Shall we have?
> For missing sequences, use ALTER SUBSCRIPTION with either REFRESH
> PUBLICATION or REFRESH PUBLICATION SEQUENCES
> For mismatched sequences, alter or re-create local sequences to have
> matching parameters as publishers.
>
> Thoughts?
>

Updated the messages as suggested.

> 6)
> postgres=# create publication pub1 for table tab1, all sequences;
> ERROR:  syntax error at or near "all"
> LINE 1: create publication pub1 for table tab1, all sequences;
>
> We can mention in commit msg that this combination is also not
> supported or what all combinations are supported. Currently it is not
> clear f

Done.
~~~~

Please find the attached v20250630 patch set addressing above comments
and other comments in [1],[2],[3] and [4].

[1] https://www.postgresql.org/message-id/CABdArM7h1qQLUb_S7i6MrLPEtHXnX%2BY2fPQaSnqhCdHktcQk5Q%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CANhcyEVbdambw%3DaVVuW0RrhQ7Lkqad%3DCdrvVA8FP6Xb%2BkP_Qzg%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CABdArM5mwL8WtGWdDdYT98ddYaB%3D3N6cfPBncvnh682X1GfbVQ%40mail.gmail.com
[4] https://www.postgresql.org/message-id/CANhcyEWKhHWFzpdAF6czbwq76NRDNCecDqQNtN6Bomn26mqHFw%40mail.gmail.com

--
Thanks,
Nisha