Horton

Thread

Commits

  1. Add ruleutils support for decompiling MERGE commands.

  1. MERGE lacks ruleutils.c decompiling support!?

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-05T13:36:41Z

    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

  2. Re: MERGE lacks ruleutils.c decompiling support!?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-05T16:21:30Z

    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/

  3. Re: MERGE lacks ruleutils.c decompiling support!?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-05T18:43:38Z

    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.

  4. Re: MERGE lacks ruleutils.c decompiling support!?

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-05T21:06:34Z

    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

  5. Re: MERGE lacks ruleutils.c decompiling support!?

    Michael Paquier <michael@paquier.xyz> — 2023-05-06T02:46:06Z

    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

  6. Re: MERGE lacks ruleutils.c decompiling support!?

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-06T02:49:15Z

    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

  7. Re: MERGE lacks ruleutils.c decompiling support!?

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-07T04:49:59Z

    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

  8. Re: MERGE lacks ruleutils.c decompiling support!?

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-07T15:02:33Z

    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

  9. Re: MERGE lacks ruleutils.c decompiling support!?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-07T17:06:09Z

    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/

  10. Re: MERGE lacks ruleutils.c decompiling support!?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-10T09:40:36Z

    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)