On Tue, Jun 23, 2026 at 3:40 PM Bharath Rupireddy <[email protected]> wrote: > > Hi, > > On Tue, Jun 16, 2026 at 6:20 PM Masahiko Sawada <[email protected]> wrote: > > > > > However, I would like to optimize this part of the code by 1/ gating > > > it with InvalidOid checkup, 2/ moving the schemaid fetch closer to > > > where it's being used i.e. inside if (!pub->alltables). I would like > > > to do this only for HEAD, so, I attached it as a separate 0002 patch. > > > For pre-HEAD versions, I removed this check and retained the > > > try_table_open() fix. > > > > I'm not sure skipping get_rel_namespace() contributes to a visible > > performance gain. Rather I'm concerned that checking > > SearchSysCacheExists2(PUBLICATIONNAMESPACEMAP) with > > schemaid=InvalidOid could lead to unexpected results. > > Thanks for reviewing! > > AFAICS it won't give unexpected results. The first time it looks up a > publication with an invalid schema OID, it would cause a cache miss > and leave a negative cache entry - but that's per publication, not per > relation, so negligible. > > That said, with the approach of using try_table_open for all tables > regardless and getting the schema from the open relation via > RelationGetNamespace, schemaid is always valid by the time it reaches > the syscache lookup (might be worth adding an assertion here). So the > get_rel_namespace optimization (0002) can be dropped entirely. > > > I'm concerned about the fact that the patch handles only the case > > where the table doesn't have the column list. The tables in the result > > not having the column list are guaranteed to be present since we do > > the existence check for them but others are not. I guess it's reliable > > if we could call try_table_open() for all tables in the table_infos, > > and check its namespace using RelationGetNamespace(). I think it > > doesn't lead to noticeable overheads in practice as we're already > > calling table_open() for all tables in ALL TABLES publications or ALL > > TABLE IN SCHEMA publications where users cannot specify the column > > list. What do you think? > > My earlier argument was that no function returning table OIDs can > guarantee they remain valid - a drop can happen right after we return > the OID, and accuracy is in the caller's hands. All the callers of > pg_get_publication_tables already handle this by JOINing with > pg_class. > > However, a closer look at other functions that either build a list of > table OIDs (expand_partitioned_rtentry) or work on previously built > table OIDs (vacuum_open_relation) proves me wrong - they all account > for concurrent table drops with try_table_open. So, I'm convinced to > add try_table_open in pg_get_publication_tables for all the tables > regardless, unless I'm missing something here.
+1 > > > > > If we use tuplestore instead of SRF, can we simplify the code as we > > > > would not need publication_tables_state and the above check? It would > > > > be only for the master, though. > > > > > > I implemented this idea for HEAD and it simplifies the code a bit. > > > > Sorry, I should have mentioned that the changes to use tuplestore > > instead of SRF should be PG20. I think that we should not such a > > refactoring even PG19. > > I will drop the tuplestore changes for now and repost them as a > refactoring patch after the PG20 dev branch is cut. > > Thoughts? +1. We can revisit the tuplestore idea for PG20, possibly in a separate thread. For bug fixing, let's focus on making it simple and less invasive. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
