| From: | Bingshuai Li <lucian1412(at)outlook(dot)com> |
|---|---|
| To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
| Cc: | vignesh C <vignesh21(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Logical Replication - revisit `is_table_publication` function implementation |
| Date: | 2026-08-20 01:40:44 |
| Message-ID: | ME0P300MB05689F50B25C44535E037962C6A42@ME0P300MB0568.AUSP300.PROD.OUTLOOK.COM |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Peter,
Thanks for the quick v3 ― the doc comments and the merged coexistence
sentences all look good.
One point from my review seems to have been dropped, though. The merged
comment in is_table_publication() now reads:
/*
* FOR TABLE cannot be used with FOR ALL TABLES or FOR ALL SEQUENCES.
*/
That states the grammar fact, but not why skipping the
pg_publication_rel scan is safe: a FOR ALL TABLES publication can still
have EXCEPT rows there, and returning false is correct precisely because
those are the only rows it can have. Would you consider keeping one more
sentence to that effect, e.g.
/*
* FOR TABLE cannot be used with FOR ALL TABLES or FOR ALL SEQUENCES.
* A FOR ALL TABLES publication can have only EXCEPT entries in
* pg_publication_rel, so it never counts as a table publication.
*/
The is_schema_publication() side is fine as written, since
pg_publication_namespace simply has no rows to find in that case.
BTW, v2 -> v3 is comment-only apart from the rebase, so the test results
I posted earlier still stand. With the comment above in place, this gets
my +1.
Regards,
Bingshuai Li
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Xuneng Zhou | 2026-08-20 01:58:28 | Re: timeout value overflow in wait for lsn |
| Previous Message | Bharath Rupireddy | 2026-08-20 01:32:00 | Re: [PATCH] Fix NULL dereference in subscription REFRESH on concurrent DROP |