At 2026-03-31 14:15:18, "王好燕 " <[email protected]> wrote: At 2026-03-31 12:08:52, "Masahiko Sawada" <[email protected]> wrote: >On Mon, Mar 30, 2026 at 12:16 AM Zhijie Hou (Fujitsu) ><[email protected]> wrote: >> >> On Friday, March 27, 2026 2:20 PM Masahiko Sawada <[email protected]> >> wrote: >> > I've attached the updated patch. I believe I've addressed all comments I >> > got >> > so far. In addition to that, I've refactored >> > is_table_publishable_in_publication() and added more regression tests. >> >> Thanks for updating the patch. >> >> The latest patch looks mostly good to me. However, I noticed one issue: the >> function returns table information even for unlogged or temporary tables. I >> think we should return NULL for those cases instead. > >Indeed. Good catch! > >> >> BTW, I think we could use is_publishable_class() as a reference to check once >> whether all unpublishable table types are properly ignored in this function. >> > >+1. I've added is_publishable_class() check. > >I've attached the updated patch. > >Regards, > >-- >Masahiko Sawada
>Amazon Web Services: https://aws.amazon.com Hi, thanks for the patch. I just reviewed v6, here comes some comments. +/* + * Similar to is_publishable_calss() but checks whether the given OID + * is a publishable "table" or not. + */ +static bool +is_publishable_table(Oid tableoid) +{ + HeapTuple tuple; + Form_pg_class relform; + + tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(tableoid)); + if (!HeapTupleIsValid(tuple)) + return false; + + relform = (Form_pg_class) GETSTRUCT(tuple); + + /* + * Sequences are publishable according to is_publishable_class() so + * explicitly exclude here. + */ + if (relform->relkind != RELKIND_SEQUENCE && + is_publishable_class(tableoid, relform)) + { + ReleaseSysCache(tuple); + return true; + } + + ReleaseSysCache(tuple); + return true; +} I think the last return should return false. + * Similar to is_publishable_calss() but checks whether the given OID “calss" should be “class”. One more nit comment on the commit message: "The existing a VARIADIC array form of pg_get_publication_tables() is" In this line, “a” seems not needed. Regards, Haoyan Wang
