On Sun, Mar 1, 2026 at 8:41 AM vignesh C <[email protected]> wrote:
>
> The attached v53 version patch has the changes for the same.
>
Few comments on 0001:
===================
1.
+ appendStringInfoString(&pubnames, quote_literal_cstr(pubname));
+ }
+
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg_plural("cannot attach table \"%s\" as partition because it is
referenced in publication \"%s\" EXCEPT clause",
+ "cannot attach table \"%s\" as partition because it is referenced
in publications \"%s\" EXCEPT clause",
Because of the quoting before the error message, the publication names
in the error message will be displayed in single quotes which are
inside double quotes.
See below examples:
ERROR: cannot attach table "testpub_root" as partition because it is
referenced in publication "'testpub8'" EXCEPT clause
ERROR: cannot attach table "testpub_root" as partition because it is
referenced in publications "'testpub8', 'testpub10'" EXCEPT clause
Do you this type of quoting single/multiple object names in error
messages at other places? What is the rationale behind this?
The other place where I see such names is following but it also
doesn't use the pattern followed above:
A.
postgres=# create subscription sub1 connection 'dbname=postgres'
publication pub9, pub10, pub11;
WARNING: publications "pub9", "pub10", "pub11" do not exist on the publisher
postgres=# create subscription sub2 connection 'dbname=postgres'
publication pub12;
WARNING: publication "pub12" does not exist on the publisher
B.
logical replication target relation "public.t10" is missing replicated
columns: "c2", "c3". For this See logicalrep_get_attrs_str and
following error_message
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg_plural("logical replication target relation \"%s.%s\" is
missing replicated column: %s",
"logical replication target relation \"%s.%s\" is missing replicated
columns: %s",
I think we should follow what is done in logicalrep_get_attrs_str for
message construction and translation.
2.
* pub_all_obj_type is one of:
*
- * TABLES
+ * TABLES [EXCEPT [TABLE] ( table [, ...] )]
Though the table is no longer optional, the above syntax reference
still shows it as optional.
3.
+ List *ancestor_exceptpuboids = GetRelationExcludedPublications(ancestor);;
One spurious semicolon at the end of above statement
4.
@@ -5838,16 +5841,22 @@ RelationBuildPublicationDesc(Relation
relation, PublicationDesc *pubdesc)
foreach(lc, ancestors)
{
Oid ancestor = lfirst_oid(lc);
+ List *ancestor_puboids = GetRelationIncludedPublications(ancestor);
+ List *ancestor_exceptpuboids = GetRelationExcludedPublications(ancestor);;
- puboids = list_concat_unique_oid(puboids,
- GetRelationPublications(ancestor));
+ puboids = list_concat_unique_oid(puboids, ancestor_puboids);
schemaid = get_rel_namespace(ancestor);
puboids = list_concat_unique_oid(puboids,
GetSchemaPublications(schemaid));
+ exceptpuboids = list_concat_unique_oid(exceptpuboids,
+ ancestor_exceptpuboids);
}
Why do we need to get the exceptpuboids for all ancestors instead of
just the top-one as we are doing in get_rel_sync_entry()?
--
With Regards,
Amit Kapila.
