MERGE lacks ruleutils.c decompiling support!?

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/

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.

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

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

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

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

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

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/

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)