Thread: MERGE lacks ruleutils.c decompiling support!?

MERGE lacks ruleutils.c decompiling support!?

From
Tom Lane
Date:
I made this function:

CREATE OR REPLACE FUNCTION test_fun()
RETURNS void
LANGUAGE SQL
BEGIN ATOMIC
MERGE INTO target
USING source s on s.id = target.id
WHEN MATCHED THEN
  UPDATE SET data = s.data
WHEN NOT MATCHED THEN
  INSERT VALUES (s.id, s.data);
end;

It appears to work fine, but:

regression=# \sf+ test_fun()
ERROR:  unrecognized query command type: 5

and it also breaks pg_dump.  Somebody screwed up pretty badly
here.  Is there any hope of fixing it for Monday's releases?

(I'd guess that decompiling the WHEN clause would take a nontrivial
amount of new code, so maybe fixing it on such short notice is
impractical.  But ugh.)

            regards, tom lane



Re: MERGE lacks ruleutils.c decompiling support!?

From
Alvaro Herrera
Date:
On 2023-May-05, Tom Lane wrote:

> I made this function:
> 
> CREATE OR REPLACE FUNCTION test_fun()
> RETURNS void
> LANGUAGE SQL
> BEGIN ATOMIC
> MERGE INTO target
> USING source s on s.id = target.id
> WHEN MATCHED THEN
>   UPDATE SET data = s.data
> WHEN NOT MATCHED THEN
>   INSERT VALUES (s.id, s.data);
> end;
> 
> It appears to work fine, but:
> 
> regression=# \sf+ test_fun()
> ERROR:  unrecognized query command type: 5
> 
> and it also breaks pg_dump.  Somebody screwed up pretty badly
> here.  Is there any hope of fixing it for Monday's releases?
> 
> (I'd guess that decompiling the WHEN clause would take a nontrivial
> amount of new code, so maybe fixing it on such short notice is
> impractical.  But ugh.)

Hmm, there is *some* code in ruleutils for MERGE, but clearly something
is missing.  Let me have a look ...

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/



Re: MERGE lacks ruleutils.c decompiling support!?

From
Alvaro Herrera
Date:
On 2023-May-05, Tom Lane wrote:

> (I'd guess that decompiling the WHEN clause would take a nontrivial
> amount of new code, so maybe fixing it on such short notice is
> impractical.  But ugh.)

Here's a first attempt.  I mostly just copied code from the insert and
update support routines.  There's a couple of things missing still, but
I'm not sure I'll get to it tonight.  I only tested to the extent of
what's in the new regression test.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

Attachment

Re: MERGE lacks ruleutils.c decompiling support!?

From
Tom Lane
Date:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Here's a first attempt.  I mostly just copied code from the insert and
> update support routines.  There's a couple of things missing still, but
> I'm not sure I'll get to it tonight.  I only tested to the extent of
> what's in the new regression test.

I did a bit of review and more work on this:

* Added the missing OVERRIDING support

* Played around with the pretty-printing indentation

* Improved test coverage, and moved the test to rules.sql where
I thought it fit more naturally.

I think we could probably commit this, though I've not tried it
in v15 yet.

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 461735e84f..60f9d08d5d 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -411,6 +411,8 @@ static void get_update_query_targetlist_def(Query *query, List *targetList,
                                             RangeTblEntry *rte);
 static void get_delete_query_def(Query *query, deparse_context *context,
                                  bool colNamesVisible);
+static void get_merge_query_def(Query *query, deparse_context *context,
+                                bool colNamesVisible);
 static void get_utility_query_def(Query *query, deparse_context *context);
 static void get_basic_select_query(Query *query, deparse_context *context,
                                    TupleDesc resultDesc, bool colNamesVisible);
@@ -5448,6 +5450,10 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
             get_delete_query_def(query, &context, colNamesVisible);
             break;

+        case CMD_MERGE:
+            get_merge_query_def(query, &context, colNamesVisible);
+            break;
+
         case CMD_NOTHING:
             appendStringInfoString(buf, "NOTHING");
             break;
@@ -7044,6 +7050,128 @@ get_delete_query_def(Query *query, deparse_context *context,
 }


+/* ----------
+ * get_merge_query_def                - Parse back a MERGE parsetree
+ * ----------
+ */
+static void
+get_merge_query_def(Query *query, deparse_context *context,
+                    bool colNamesVisible)
+{
+    StringInfo    buf = context->buf;
+    RangeTblEntry *rte;
+    ListCell   *lc;
+
+    /* Insert the WITH clause if given */
+    get_with_clause(query, context);
+
+    /*
+     * Start the query with MERGE INTO relname
+     */
+    rte = rt_fetch(query->resultRelation, query->rtable);
+    Assert(rte->rtekind == RTE_RELATION);
+    if (PRETTY_INDENT(context))
+    {
+        appendStringInfoChar(buf, ' ');
+        context->indentLevel += PRETTYINDENT_STD;
+    }
+    appendStringInfo(buf, "MERGE INTO %s%s",
+                     only_marker(rte),
+                     generate_relation_name(rte->relid, NIL));
+
+    /* Print the relation alias, if needed */
+    get_rte_alias(rte, query->resultRelation, false, context);
+
+    /* Print the source relation and join clause */
+    get_from_clause(query, " USING ", context);
+    appendContextKeyword(context, " ON ",
+                         -PRETTYINDENT_STD, PRETTYINDENT_STD, 2);
+    get_rule_expr(query->jointree->quals, context, false);
+
+    /* Print each merge action */
+    foreach(lc, query->mergeActionList)
+    {
+        MergeAction *action = lfirst_node(MergeAction, lc);
+
+        appendContextKeyword(context, " WHEN ",
+                             -PRETTYINDENT_STD, PRETTYINDENT_STD, 2);
+        appendStringInfo(buf, "%sMATCHED", action->matched ? "" : "NOT ");
+
+        if (action->qual)
+        {
+            appendContextKeyword(context, " AND ",
+                                 -PRETTYINDENT_STD, PRETTYINDENT_STD, 3);
+            get_rule_expr(action->qual, context, false);
+        }
+        appendContextKeyword(context, " THEN ",
+                             -PRETTYINDENT_STD, PRETTYINDENT_STD, 3);
+
+        if (action->commandType == CMD_INSERT)
+        {
+            /* This generally matches get_insert_query_def() */
+            List       *strippedexprs = NIL;
+            const char *sep = "";
+            ListCell   *lc2;
+
+            appendStringInfoString(buf, "INSERT");
+
+            if (action->targetList)
+                appendStringInfoString(buf, " (");
+            foreach(lc2, action->targetList)
+            {
+                TargetEntry *tle = (TargetEntry *) lfirst(lc2);
+
+                Assert(!tle->resjunk);
+
+                appendStringInfoString(buf, sep);
+                sep = ", ";
+
+                appendStringInfoString(buf,
+                                       quote_identifier(get_attname(rte->relid,
+                                                                    tle->resno,
+                                                                    false)));
+                strippedexprs = lappend(strippedexprs,
+                                        processIndirection((Node *) tle->expr,
+                                                           context));
+            }
+            if (action->targetList)
+                appendStringInfoChar(buf, ')');
+
+            if (action->override)
+            {
+                if (action->override == OVERRIDING_SYSTEM_VALUE)
+                    appendStringInfoString(buf, " OVERRIDING SYSTEM VALUE");
+                else if (action->override == OVERRIDING_USER_VALUE)
+                    appendStringInfoString(buf, " OVERRIDING USER VALUE");
+            }
+
+            if (strippedexprs)
+            {
+                appendContextKeyword(context, " VALUES (",
+                                     -PRETTYINDENT_STD, PRETTYINDENT_STD, 4);
+                get_rule_list_toplevel(strippedexprs, context, false);
+                appendStringInfoChar(buf, ')');
+            }
+            else
+                appendStringInfoString(buf, " DEFAULT VALUES");
+        }
+        else if (action->commandType == CMD_UPDATE)
+        {
+            appendStringInfoString(buf, "UPDATE SET ");
+            get_update_query_targetlist_def(query, action->targetList,
+                                            context, rte);
+        }
+        else if (action->commandType == CMD_DELETE)
+            appendStringInfoString(buf, "DELETE");
+        else if (action->commandType == CMD_NOTHING)
+            appendStringInfoString(buf, "DO NOTHING");
+    }
+
+    /* No RETURNING support in MERGE yet */
+    Assert(query->returningList == NIL);
+}
+
+
 /* ----------
  * get_utility_query_def            - Parse back a UTILITY parsetree
  * ----------
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 69957687fe..684ecc2ed7 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3585,6 +3585,91 @@ MERGE INTO rule_merge2 t USING (SELECT 1 AS a) s
         DELETE
     WHEN NOT MATCHED THEN
         INSERT VALUES (s.a, '');
+-- test deparsing
+CREATE TABLE sf_target(id int, data text, filling int[]);
+CREATE FUNCTION merge_sf_test()
+ RETURNS void
+ LANGUAGE sql
+BEGIN ATOMIC
+ MERGE INTO sf_target t
+   USING rule_merge1 s
+   ON (s.a = t.id)
+WHEN MATCHED
+   AND (s.a + t.id) = 42
+   THEN UPDATE SET data = repeat(t.data, s.a) || s.b, id = length(s.b)
+WHEN NOT MATCHED
+   AND (s.b IS NOT NULL)
+   THEN INSERT (data, id)
+   VALUES (s.b, s.a)
+WHEN MATCHED
+   AND length(s.b || t.data) > 10
+   THEN UPDATE SET data = s.b
+WHEN MATCHED
+   AND s.a > 200
+   THEN UPDATE SET filling[s.a] = t.id
+WHEN MATCHED
+   AND s.a > 100
+   THEN DELETE
+WHEN MATCHED
+   THEN DO NOTHING
+WHEN NOT MATCHED
+   AND s.a > 200
+   THEN INSERT DEFAULT VALUES
+WHEN NOT MATCHED
+   AND s.a > 100
+   THEN INSERT (id, data) OVERRIDING SYSTEM VALUE
+   VALUES (s.a, DEFAULT)
+WHEN NOT MATCHED
+   AND s.a > 0
+   THEN INSERT
+   VALUES (s.a, s.b, DEFAULT)
+WHEN NOT MATCHED
+   THEN INSERT (filling[1], id)
+   VALUES (s.a, s.a);
+END;
+\sf merge_sf_test
+CREATE OR REPLACE FUNCTION public.merge_sf_test()
+ RETURNS void
+ LANGUAGE sql
+BEGIN ATOMIC
+ MERGE INTO sf_target t
+    USING rule_merge1 s
+    ON (s.a = t.id)
+    WHEN MATCHED
+     AND ((s.a + t.id) = 42)
+     THEN UPDATE SET data = (repeat(t.data, s.a) || s.b), id = length(s.b)
+    WHEN NOT MATCHED
+     AND (s.b IS NOT NULL)
+     THEN INSERT (data, id)
+      VALUES (s.b, s.a)
+    WHEN MATCHED
+     AND (length((s.b || t.data)) > 10)
+     THEN UPDATE SET data = s.b
+    WHEN MATCHED
+     AND (s.a > 200)
+     THEN UPDATE SET filling[s.a] = t.id
+    WHEN MATCHED
+     AND (s.a > 100)
+     THEN DELETE
+    WHEN MATCHED
+     THEN DO NOTHING
+    WHEN NOT MATCHED
+     AND (s.a > 200)
+     THEN INSERT DEFAULT VALUES
+    WHEN NOT MATCHED
+     AND (s.a > 100)
+     THEN INSERT (id, data) OVERRIDING SYSTEM VALUE
+      VALUES (s.a, DEFAULT)
+    WHEN NOT MATCHED
+     AND (s.a > 0)
+     THEN INSERT (id, data, filling)
+      VALUES (s.a, s.b, DEFAULT)
+    WHEN NOT MATCHED
+     THEN INSERT (filling[1], id)
+      VALUES (s.a, s.a);
+END
+DROP FUNCTION merge_sf_test;
+DROP TABLE sf_target;
 --
 -- Test enabling/disabling
 --
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 4caab3434b..9111618005 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1277,6 +1277,55 @@ MERGE INTO rule_merge2 t USING (SELECT 1 AS a) s
     WHEN NOT MATCHED THEN
         INSERT VALUES (s.a, '');

+-- test deparsing
+CREATE TABLE sf_target(id int, data text, filling int[]);
+
+CREATE FUNCTION merge_sf_test()
+ RETURNS void
+ LANGUAGE sql
+BEGIN ATOMIC
+ MERGE INTO sf_target t
+   USING rule_merge1 s
+   ON (s.a = t.id)
+WHEN MATCHED
+   AND (s.a + t.id) = 42
+   THEN UPDATE SET data = repeat(t.data, s.a) || s.b, id = length(s.b)
+WHEN NOT MATCHED
+   AND (s.b IS NOT NULL)
+   THEN INSERT (data, id)
+   VALUES (s.b, s.a)
+WHEN MATCHED
+   AND length(s.b || t.data) > 10
+   THEN UPDATE SET data = s.b
+WHEN MATCHED
+   AND s.a > 200
+   THEN UPDATE SET filling[s.a] = t.id
+WHEN MATCHED
+   AND s.a > 100
+   THEN DELETE
+WHEN MATCHED
+   THEN DO NOTHING
+WHEN NOT MATCHED
+   AND s.a > 200
+   THEN INSERT DEFAULT VALUES
+WHEN NOT MATCHED
+   AND s.a > 100
+   THEN INSERT (id, data) OVERRIDING SYSTEM VALUE
+   VALUES (s.a, DEFAULT)
+WHEN NOT MATCHED
+   AND s.a > 0
+   THEN INSERT
+   VALUES (s.a, s.b, DEFAULT)
+WHEN NOT MATCHED
+   THEN INSERT (filling[1], id)
+   VALUES (s.a, s.a);
+END;
+
+\sf merge_sf_test
+
+DROP FUNCTION merge_sf_test;
+DROP TABLE sf_target;
+
 --
 -- Test enabling/disabling
 --

Re: MERGE lacks ruleutils.c decompiling support!?

From
Michael Paquier
Date:
On Fri, May 05, 2023 at 05:06:34PM -0400, Tom Lane wrote:
> I did a bit of review and more work on this:
>
> * Added the missing OVERRIDING support
>
> * Played around with the pretty-printing indentation
>
> * Improved test coverage, and moved the test to rules.sql where
> I thought it fit more naturally.
>
> I think we could probably commit this, though I've not tried it
> in v15 yet.

Seems rather OK..

+WHEN NOT MATCHED
+   AND s.a > 100
+   THEN INSERT (id, data) OVERRIDING SYSTEM VALUE
+   VALUES (s.a, DEFAULT)

About OVERRIDING, I can see that this is still missing coverage for
OVERRIDING USER VALUE.
--
Michael

Attachment

Re: MERGE lacks ruleutils.c decompiling support!?

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> +WHEN NOT MATCHED
> +   AND s.a > 100
> +   THEN INSERT (id, data) OVERRIDING SYSTEM VALUE
> +   VALUES (s.a, DEFAULT)

> About OVERRIDING, I can see that this is still missing coverage for
> OVERRIDING USER VALUE.

Yeah, I couldn't see that covering that too was worth any cycles.

            regards, tom lane



Re: MERGE lacks ruleutils.c decompiling support!?

From
Tom Lane
Date:
I wrote:
> I think we could probably commit this, though I've not tried it
> in v15 yet.

Ping?  The hours grow short, if we're to get this into 15.3.

            regards, tom lane



Re: MERGE lacks ruleutils.c decompiling support!?

From
Tom Lane
Date:
I wrote:
> Ping?  The hours grow short, if we're to get this into 15.3.

I went ahead and pushed v2, since we can't wait any longer if
we're to get reasonable buildfarm coverage before 15.3 wraps.

            regards, tom lane



Re: MERGE lacks ruleutils.c decompiling support!?

From
Alvaro Herrera
Date:
On 2023-May-07, Tom Lane wrote:

> I wrote:
> > Ping?  The hours grow short, if we're to get this into 15.3.
> 
> I went ahead and pushed v2, since we can't wait any longer if
> we're to get reasonable buildfarm coverage before 15.3 wraps.

Much appreciated.  I wanted to get back to this yesterday but was unable
to.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/



Re: MERGE lacks ruleutils.c decompiling support!?

From
Alvaro Herrera
Date:
BTW, I spent some time adding a cross-check to see if the code was at
least approximately correct for all the queries in the MERGE regression
tests, and couldn't find any failures.  I then extended the test to the
other optimizable commands, and couldn't find any problems there either.

My approach was perhaps a bit simple-minded: I just patched
pg_analyze_and_rewrite_fixedparams() to call pg_get_querydef() after
parse_analyze_fixedparams(), then ran the main regression tests.  No
crashes.  Also had it output as a WARNING together with the
query_string, so that I could eyeball for any discrepancies; I couldn't
find any queries that produce wrong contents, though this was just
manual examination of the resulting noise logs.

I suppose a better approach might be to run the query produced by
pg_get_querydef() again through parse analysis and see if it produces
the same; or better yet, discard the original parsed query and parse the
pg_get_querydef().  I didn't try to do this.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No deja de ser humillante para una persona de ingenio saber
que no hay tonto que no le pueda enseñar algo." (Jean B. Say)