Hi Peter, > On Mon, 16 Mar 2026 at 17:43, Peter Eisentraut <[email protected]> wrote: > > > > On 11.03.26 08:34, Ashutosh Bapat wrote: > > > There are two new patches 0004 and 0005 in the attached patchset. > > > > I have committed this, including the 0004 patch.
Thanks a lot. > > Let's consider the > > 0005 patch separately. Will share the rebased patch soon. This thread may see discussion about the commit itself. Should I start a new thread for 0005 or use this one? New one seems better to me with a new CF entry. > > > > The buildfarm shows some instability in the pg_upgrade test, because > > labels are printed by pg_get_propgraphdef() in implementation-dependent > > order. Attached is a quick patch to sort the labels before printing. > > Check please. The patch looks fine to me. While reviewing it, I noticed that the function has an extra loop to count the number of variables. I don't think it's needed. The count can be obtained from the list length. In the attached patch, I have removed that loop. Am I missing something? 0001 is your patch 0002 removes the loop + some cosmetic changes Hi Kirill, > Do we need to keep relation lock until end of function > (table_close(pglrel, AccessShareLock);)? I think you are right. Fixed in the attached. > I'm not sure if list_sort is > interruptible. I don't think it matters here. It will be very rare, if not impossible, to have so many labels as to let the sorting run for milliseconds together. The foreach loop afterwards is also not interruptible. Any reason you think it should be interruptible? -- Best Wishes, Ashutosh Bapat
From 93f28ca2d79f325a5ef81cfca5e4b899ed717486 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Mon, 16 Mar 2026 13:39:57 +0100 Subject: [PATCH v20260316 1/2] WIP: Dump labels in reproducible order --- src/backend/utils/adt/ruleutils.c | 43 ++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 153a92fd3ea..dc736c6bd37 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -1772,6 +1772,21 @@ make_propgraphdef_elements(StringInfo buf, Oid pgrelid, char pgekind) table_close(pgerel, AccessShareLock); } +struct oid_str_pair +{ + Oid oid; + char *str; +}; + +static int +list_oid_str_pair_cmp_by_str(const ListCell *p1, const ListCell *p2) +{ + struct oid_str_pair *v1 = lfirst(p1); + struct oid_str_pair *v2 = lfirst(p2); + + return strcmp(v1->str, v2->str); +} + /* * Generates label and properties list. Pass in the element OID, the element * alias, and the graph relation OID. Result is appended to buf. @@ -1784,6 +1799,7 @@ make_propgraphdef_labels(StringInfo buf, Oid elid, const char *elalias, Oid elre SysScanDesc scan; int count; HeapTuple tup; + List *label_list = NIL; pglrel = table_open(PropgraphElementLabelRelationId, AccessShareLock); @@ -1800,29 +1816,42 @@ make_propgraphdef_labels(StringInfo buf, Oid elid, const char *elalias, Oid elre } systable_endscan(scan); + /* + * Collect the labels into a list, then sort before printing, to get stable dump output. + */ + scan = systable_beginscan(pglrel, PropgraphElementLabelElementLabelIndexId, true, NULL, 1, scankey); while ((tup = systable_getnext(scan))) { Form_pg_propgraph_element_label pgelform = (Form_pg_propgraph_element_label) GETSTRUCT(tup); - const char *labelname; + struct oid_str_pair *osp; + + osp = palloc_object(struct oid_str_pair); + osp->oid = pgelform->oid; + osp->str = get_propgraph_label_name(pgelform->pgellabelid); - labelname = get_propgraph_label_name(pgelform->pgellabelid); + label_list = lappend(label_list, osp); + } + + systable_endscan(scan); - if (strcmp(labelname, elalias) == 0) + list_sort(label_list, list_oid_str_pair_cmp_by_str); + + foreach_ptr(struct oid_str_pair, osp, label_list) + { + if (strcmp(osp->str, elalias) == 0) { /* If the default label is the only label, don't print anything. */ if (count != 1) appendStringInfo(buf, " DEFAULT LABEL"); } else - appendStringInfo(buf, " LABEL %s", quote_identifier(labelname)); + appendStringInfo(buf, " LABEL %s", quote_identifier(osp->str)); - make_propgraphdef_properties(buf, pgelform->oid, elrelid); + make_propgraphdef_properties(buf, osp->oid, elrelid); } - systable_endscan(scan); - table_close(pglrel, AccessShareLock); } base-commit: cd8844e7db64d57554c780ec9881457c573c51ab -- 2.34.1
From 5de11fde244608100d76ba92c642f986877d0455 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Mon, 16 Mar 2026 21:10:28 +0530 Subject: [PATCH v20260316 2/2] pg_indent and review changes --- src/backend/utils/adt/ruleutils.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index dc736c6bd37..07617eb4a00 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -1774,8 +1774,8 @@ make_propgraphdef_elements(StringInfo buf, Oid pgrelid, char pgekind) struct oid_str_pair { - Oid oid; - char *str; + Oid oid; + char *str; }; static int @@ -1808,18 +1808,10 @@ make_propgraphdef_labels(StringInfo buf, Oid elid, const char *elalias, Oid elre BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(elid)); - count = 0; - scan = systable_beginscan(pglrel, PropgraphElementLabelElementLabelIndexId, true, NULL, 1, scankey); - while ((tup = systable_getnext(scan))) - { - count++; - } - systable_endscan(scan); - /* - * Collect the labels into a list, then sort before printing, to get stable dump output. + * Collect the labels into a list, then sort before printing, to get + * stable dump output. */ - scan = systable_beginscan(pglrel, PropgraphElementLabelElementLabelIndexId, true, NULL, 1, scankey); while ((tup = systable_getnext(scan))) @@ -1835,7 +1827,20 @@ make_propgraphdef_labels(StringInfo buf, Oid elid, const char *elalias, Oid elre } systable_endscan(scan); + table_close(pglrel, AccessShareLock); + count = list_length(label_list); + + /* + * Each element has at least one label. It has default label if no + * declared label exists. + */ + Assert(count > 0); + + /* + * It is enough for the comparison function to compare just labels, since + * all the labels of a element table should have distinct names. + */ list_sort(label_list, list_oid_str_pair_cmp_by_str); foreach_ptr(struct oid_str_pair, osp, label_list) @@ -1851,8 +1856,6 @@ make_propgraphdef_labels(StringInfo buf, Oid elid, const char *elalias, Oid elre make_propgraphdef_properties(buf, osp->oid, elrelid); } - - table_close(pglrel, AccessShareLock); } /* -- 2.34.1
