Re: Logical Replication of sequences

On Thu, 24 Jul 2025 at 13:12, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Vignesh,
>
> Thanks for working the project. Here are my comments only for 0001 and 0002.
> Sorry if my points have already been discussed, this thread is too huge to catchup for me :-(.
>
> 01.
> Do we have to document the function and open to users? Previously it was not.
> Another example is pg_get_publication_tables, which is used only by the backend.

Now we are exposing this function to the user, this LSN value will be
used by the user to find out if the sequence has changed compared to
the subscriber and decide if it should be synchronized or not.

> 02.
> ```
> SELECT last_value, is_called, log_cnt FROM pg_get_sequence_data('test_seq1');
> ```
>
> I came up with the way that the function returns page_lsn, how do you feel?
>
> ```
> SELECT last_value, is_called, log_cnt, page_lsn <= pg_current_wal_lsn() as lsn FROM pg_get_sequence_data('test_seq1');
> ```

Modified

> 03.
> Regarding the tab completion on psql. When I input till "CREATE PUBLICATION pub FOR ALL TABLES"
> and push the tab, the word "WITH" was completed. But I'm afraid it may be too
> much because users can input like "FOR ALL TABLES, ALL SEQUENCES". Should we
> suggest the word ", ALL SEQUENCES" here or we can ignore?
>
> Same point can be said when "FOR ALL SEQUENCES" was input.

This was discussed earlier at [1]:
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;

> 04.
> Same as above, "WITH" could be completed when I input till "FOR ALL SEQUENCES".
> However, current patch rejects the WITH clause for ALL SEQUENCE publication.
> How do you feel? I'm wondering we should stop the suggestion.

Modified

> 05.
> is_publishable_class() has a comment atop the function:
> ```
> * Does same checks as check_publication_add_relation() above, but does not
> * need relation to be opened and also does not throw errors.
> ```
>
> But this is not correct becasue check_publication_add_relation() does not allow
> sequences. Can you modify the comment accordingly?

Modified

> 06.
> ```
> values[Anum_pg_publication_puballtables - 1] = stmt->for_all_tables;
> values[Anum_pg_publication_puballsequences - 1] = stmt->for_all_sequences;
> ```
>
> They should be passed as Datum like others.

Modified

> 07.
> ```
> + /*
> + * indicates that this is special publication which should encompass all
> + * sequences in the database (except for the unlogged and temp ones)
> + */
> + bool puballsequences;
> ```
>
> Let me my thought here. I initially wondered whether we should synchronize
> unlogged sequences, because it is actually possible. However, I found the article[1]
> that primal use-case of the unlogged sequence is to associate with the unlogged table.
> Based on the point, I agree not to include such sequences - they might be a leaked
> sequence.

I agree

> 08.
> ```
> @@ -1902,6 +1952,13 @@ PublicationDropTables(Oid pubid, List *rels, bool missing_ok)
> errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("column list must not be specified in ALTER PUBLICATION ... DROP"));
>
> + if (RelationGetForm(rel)->relkind == RELKIND_SEQUENCE)
> + ereport(ERROR,
> + errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("relation \"%s\" is not part of the publication",
> + RelationGetRelationName(rel)),
> + errdetail_relkind_not_supported(RelationGetForm(rel)->relkind));
> +
> ```
>
> Hmm, I feel this is not needed because in the first place sequences cannot be
> specified as the target. Other objects like view is not handled here.

Modified

> 09.
> ```
> + StringInfo pub_type;
> +
> Form_pg_publication pubform = (Form_pg_publication) GETSTRUCT(tup);
> ```
>
> I feel this blank is not needed.

Modified

> 10.
> ```
> /*
> * Process all_objects_list to set all_tables/all_sequences.
> * Also, checks if the pub_object_type has been specified more than once.
> */
> static void
> preprocess_pub_all_objtype_list(List *all_objects_list, bool *all_tables,
> bool *all_sequences, core_yyscan_t yyscanner)
> ```
>
> I'm not a native speaker, but I feel object list is "process"'d here, not
> "preprocess"'d. Can we rename to process_pub_all_objtype_list?

Since we have an existing function preprocess_pubobj_list which uses
preprocess, keeping it as preprocess to maintain consistency

> 11.
> ```
> + /* The WITH clause is not applicable to FOR ALL SEQUENCES publications */
> + if (!pubinfo->puballsequences || pubinfo->puballtables)
> ```
>
> This meant that FOR ALL TABLES/SEQUENCES publication without any options can be
> dumped like:
>
> ```
> CREATE PUBLICATION pub FOR ALL TABLES, ALL SEQUENCES WITH (publish = 'insert, update, delete, truncate')
> ```
>
> However, since the WITH clause is set with ALL SEQUENCE, generated script could
> raise the WARNING, which may be confusing for users:
>
> ```
> $ psql -U postgres -f dump.sql
> ...
> psql:dump.sql:24: WARNING: WITH clause parameters do not affect sequence synchronization
> ```
>
> One workaround for it is not to output WITH cause for default setting. Thought?

I felt this is ok, it will make users aware that for sequence this is
not applicable.

The attached v20250724 version patch has the changes for the same.
[1] - https://www.postgresql.org/message-id/CABdArM5axwoTorZnJww5rE79SNzvnnXCfWkv7XJex1Rkz%3DJDog%40mail.gmail.com

Regards,
Vignesh