From: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
---|---|
To: | Richard Guo <guofenglinux(at)gmail(dot)com> |
Cc: | Joe Wildish <joe(at)lateraljoin(dot)com>, Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: MERGE bug report |
Date: | 2022-04-08 21:26:38 |
Message-ID: | 202204082126.34hklz6x5rh4@alvherre.pgsql |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2022-Apr-06, Richard Guo wrote:
> That's right. The varattno is set to zero for whole-row Var. And in this
> case these whole-row Vars are not included in the targetlist.
>
> Attached is an attempt for the fix.
Wow, this is very interesting. I was surprised that this patch was
necessary at all -- I mean, if wholerow refs don't work, then why do
references to any other columns work? The answer is that parse_merge.c
is already setting up the subplan's targetlist by expanding all vars of
the source relation. I then remembered than in Simon's (or Pavan's)
original coding, parse_merge.c had a hack to include a var with the
source's wholerow in that targetlist, which I had later removed ...
I eventually realized that there's no need for parse_merge.c to expand
the source rel at all, and indeed it's wasteful: we can just let
preprocess_targetlist include the vars that are referenced by either
quals or each action's targetlist instead. That led me to the attached
patch, which is not commit-quality yet but it should show what I have in
mind.
I added a test query to tickle this problematic case.
Another point, not completely connected to this bug but appearing in the
same function, is that we have some redundant code: we can just let the
stanza for UPDATE/DELETE do the identity columns dance. This saves a
few lines in the MERGE-specific stanza there, which was doing exactly
the same thing. (There's a difference in the "inh" test, but I think
that was just outdated.)
I also discovered that the comment for fix_join_expr needed an update,
since it doesn't mention MERGE, and it does mention all other situations
in which it is used. Added that too.
This patch is a comment about "aggregates, window functions and
placeholder vars". This was relevant and correct when only the qual of
each action was being handled (i.e., Richard's patch). Now that we're
also handling the action's targetlist, I think I need to put the PVC
flags back. But no tests broke, which probably means we also need some
additional tests cases.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Attachment | Content-Type | Size |
---|---|---|
fix-merge-targetlist.patch | text/x-diff | 6.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Geoghegan | 2022-04-08 21:43:31 | Re: Lowering the ever-growing heap->pd_lower |
Previous Message | Peter Geoghegan | 2022-04-08 21:19:50 | Re: Lowering the ever-growing heap->pd_lower |