Re: Logical Replication of sequences

On Tue, 7 Oct 2025 at 12:09, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Oct 7, 2025 at 10:53 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > 2)
> > postgres=# create publication pub1 for all sequences WITH(publish='insert');
> > ERROR: publication parameters are not supported for publications
> > defined as FOR ALL SEQUENCES
> >
> > postgres=# alter publication pub1 add table tab1;
> > ERROR: Tables or sequences cannot be added to or dropped from
> > publication defined FOR ALL TABLES, ALL SEQUENCES, or both
> >
> > a) First msg has 'as', while second does not. Shall we make both the
> > same? I think we can get rid of 'as'.
> > b) Shouldn't the error msg start with lower case (second one)?
> >
>
> The (b) is related to following change:
>
> +  if (tables && (pubform->puballtables || pubform->puballsequences))
> ereport(ERROR,
> -        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> -         errmsg("publication \"%s\" is defined as FOR ALL TABLES",
> -            NameStr(pubform->pubname)),
> -         errdetail("Tables cannot be added to or dropped from FOR ALL
> TABLES publications.")));
> +        errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +        errmsg("Tables or sequences cannot be added to or dropped
> from publication defined FOR ALL TABLES, ALL SEQUENCES, or both"));
> }
>
> I see a bigger problem where we made errdetail as errmsg. Here, we are
> trying to combine the message FOR ALL TABLES and FOR ALL SEQUENCES
> which caused this change/confusion. It is better to keep them separate
> to avoid confusion. The similar problem exists for following message:
> - if (schemaidlist && pubform->puballtables)
> + if (schemaidlist && (pubform->puballtables || pubform->puballsequences))
> ereport(ERROR,
> - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> - errmsg("publication \"%s\" is defined as FOR ALL TABLES",
> - NameStr(pubform->pubname)),
> - errdetail("Schemas cannot be added to or dropped from FOR ALL TABLES
> publications.")));
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("Schemas cannot be added to or dropped from publication
> defined FOR ALL TABLES, ALL SEQUENCES, or both"));

Modified to keep it as separate error messages.

> If we fix both of these then we don't need to do anything for point (a).

Agreed

> Few other comments:
> =================
> 1. Is there a reason not to change just the footers part in
> describeOneTableDetails()?

I tried the approach to keep it as a footer and it simplifies the code
further. Update the patch accordingly.

> 2. I think we can move create_publication.sgml and
> pg_publication_sequences related changes to 0001 from doc patch.

Modified

> 3. Atop is_publishable_class(), we mention it has all the checks as
> check_publication_add_relation but the sequence check is different. Is
> it because check_publication_add_relation() is not called FOR ALL
> SEQUENCES? If so, I have modified a few comments in the attached
> related to that.

Updated

> 4.
> @@ -878,13 +885,35 @@ CreatePublication(ParseState *pstate,
> CreatePublicationStmt *stmt)
> &publish_via_partition_root_given,
> &publish_via_partition_root,
> &publish_generated_columns_given,
> - &publish_generated_columns);
> + &publish_generated_columns,
> + def_pub_action);
> +
> + if (stmt->for_all_sequences &&
> + (publish_given || publish_via_partition_root_given ||
> + publish_generated_columns_given))
> + {
> + /*
> + * WITH clause parameters are not applicable when creating a FOR ALL
> + * SEQUENCES publication. If the publication includes tables as well,
> + * issue a notice.
> + */
> + if (!stmt->for_all_tables)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("publication parameters are not supported for publications
> defined as FOR ALL SEQUENCES"));
> +
> + ereport(NOTICE,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("publication parameters are not applicable to sequence
> synchronization and will be ignored"));
> + }
>
> This change looks a bit ad hoc to me. I think it would be better to
> handle this inside parse_publication_options(). The function can take
> the third parameter as for_all_sequences and then use that to give
> error when any options are present.

Modified

Thanks for the comments, the attached patch has the changes for the same.

Regards,
Vignesh