On Fri, Feb 10, 2023 at 4:36 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Feb 9, 2023 at 3:25 PM Ajin Cherian <itsa...@gmail.com> wrote: > > > > Comments on 0001 and 0002 > ======================= >
Few more comments on 0001 and 0003 =============================== 1. I think having 'internal' in an exposed function pg_get_viewdef_internal() seems a bit odd to me. Shall we name it something like pg_get_viewdef_sys() as it consults the system cache? 2. In pg_get_trigger_whenclause(), there are various things that have changed in the new code but the patch didn't update those. It is important to update those especially because it replaces the existing code as well. For example, it should use GET_PRETTY_FLAGS for prettyFlags, then some variables are not initialized, and also didn't use rellockmode for old and new rtes. I suggest carefully comparing the code with the corresponding existing code in the function pg_get_triggerdef_worker(). 3. deparse_CreateTrigStmt { ... + + if (node->deferrable) + list = lappend(list, new_string_object("DEFERRABLE")); + if (node->initdeferred) + list = lappend(list, new_string_object("INITIALLY DEFERRED")); + append_array_object(ret, "%{constraint_attrs: }s", list); ... } Is there a reason that the above part of the conditions doesn't match the below conditions in pg_get_triggerdef_worker()? pg_get_triggerdef_worker() { ... if (!trigrec->tgdeferrable) appendStringInfoString(&buf, "NOT "); appendStringInfoString(&buf, "DEFERRABLE INITIALLY "); if (trigrec->tginitdeferred) appendStringInfoString(&buf, "DEFERRED "); else appendStringInfoString(&buf, "IMMEDIATE "); ... } 4. In deparse_CreateTrigStmt(), the handling for REFERENCING OLD/NEW Table is missing. See the corresponding code in pg_get_triggerdef_worker(). 5. In deparse_CreateTrigStmt(), the function name for EXECUTE PROCEDURE is generated in a different way as compared to what we are doing in pg_get_triggerdef_worker(). Is there a reason for the same? 6. +char * +pg_get_partkeydef_simple(Oid relid) +{ + return pg_get_partkeydef_worker(relid, 0, false, false); +} The 0 is not a valid value for prettyFlags, so not sure what is the intention here. I think you need to use GET_PRETTY_FLAGS() here. 7. The above comment applies to pg_get_constraintdef_command_simple() as well. 8. Can we think of better names instead of appending 'simple' in the above two cases? -- With Regards, Amit Kapila.