On Thu, Jul 30, 2026 at 6:15 PM Bharath Rupireddy <[email protected]> wrote: > > Hi, > > On Sun, May 31, 2026 at 11:30 PM Ajin Cherian <[email protected]> wrote: > > > > I've reviewed the patch and it looks good > > Thanks Satya for the off-list discussion, and thanks Ajin for the review. > > > just a small comment - instead of silently continuing after finding a > > dropped local table, would you want to log an error message? > > IMHO, logging a message is unnecessary. The origin check only raises a > WARNING and doesn't drive the actual refresh, so silently skipping a > concurrently dropped relation is harmless. > > A similar fix for concurrent relation drops in > pg_get_publication_tables() is in commit 63e7a0d2c, and I'm following > it in using try_table_open() and in not logging for dropped local > relations. > > Please find attached the v2 patch. It has the following changes:
Thank you for updating the patch. > 1/ Uses try_table_open() for all relations and gets the namespace > using RelationGetNamespace(). I'm not sure that this patch should follow the changes commit 63e7a0d2c did for pg_get_publication_tables() since that function opened tables if the column list is not specified, meaning that for a FOR ALL TABLES publication it opened all tables, even before the fix. On the other hand, check_publications_origin_tables() doesn't do so. I think we should carefully consider that acquiring AccessShareLock on all tables could introduce measurable overheads in execution time as well as relcache bloat. A plain NULL check might work better here like the v1 patch does. Even with the v1 patch, we could end up getting a mismatched schema name and table name due to concurrent schema name change or changing schema of the table. But the result would not be serious; it just ends up adding a filter that could exclude unexpected tables and missing some WARNINGs. > 2/ Deduplicates the common code that quotes the subscription > relations' schema-qualified names for tables and sequences into a > helper function. While it works for v19 and master, it doesn't work for v16, v17, and v18. ISTM it doesn't help simplify the code that much. I guess we can live with such a duplication. > 3/ Adds a TAP test with an injection point in 0002 (which I don't > intend to be committed). Thank you for creating the test case. I agree that the test for this issue needs a new injection point and it would not be good for back branches. I still see value a bit in having the regression test only for HEAD. Feedback is welcome. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
