Thread
Commits
-
Add support for MERGE SQL command
- 7103ebb7aae8 15.0 landed
-
Avoid repeated name lookups during table and index DDL.
- 5f173040e324 9.4.0 cited
-
Use an MVCC snapshot, rather than SnapshotNow, for catalog scans.
- 568d4138c646 9.4.0 cited
-
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-03-22T23:13:16Z
On Thu, Mar 22, 2018 at 11:42 AM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > A slightly improved version attached. You still need to remove this change: > diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h > index a4574cd533..dbfb5d2a1a 100644 > --- a/src/include/miscadmin.h > +++ b/src/include/miscadmin.h > @@ -444,5 +444,6 @@ extern bool has_rolreplication(Oid roleid); > /* in access/transam/xlog.c */ > extern bool BackupInProgress(void); > extern void CancelBackup(void); > +extern int64 GetXactWALBytes(void); I see that we're still using two target RTEs in this latest revision, v23e -- the new approach to partitioning, which I haven't had time to study in more detail, has not produced a change there. This causes weird effects, such as the following: """ pg@~[20658]=# create table foo(bar int4); CREATE TABLE pg@~[20658]=# merge into foo f using (select 1 col) dd on f.bar=dd.col when matched then update set bar = f.barr + 1; ERROR: column f.barr does not exist LINE 1: ...n f.bar=dd.col when matched then update set bar = f.barr + 1... ^ HINT: Perhaps you meant to reference the column "f.bar" or the column "f.bar". """ While I think this this particular HINT buglet is pretty harmless, I continue to be concerned about the unintended consequences of having multiple RTEs for MERGE's target table. Each RTE comes from a different lookup path -- the first one goes through setTargetTable()'s parserOpenTable() + addRangeTableEntryForRelation() calls. The second one goes through transformFromClauseItem(), for the join, which ultimately ends up calling transformTableEntry()/addRangeTableEntry(). Consider commit 5f173040, which fixed a privilege escalation security bug around multiple name lookup. Could the approach taken by MERGE here introduce a similar security issue? Using GDB, I see two calls to RangeVarGetRelidExtended() when a simple MERGE is executed. They both have identical relation arguments, that look like this: (gdb) p *relation $4 = { type = T_RangeVar, catalogname = 0x0, schemaname = 0x0, relname = 0x5600ebdcafb0 "foo", inh = 1 '\001', relpersistence = 112 'p', alias = 0x5600ebdcb048, location = 11 } This seems like something that needs to be explained, at a minimum. Even if I'm completely wrong about there being a security hazard, maybe the suggestion that there might be still gives you some idea of what I mean about unintended consequences. -- Peter Geoghegan -
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-03-23T11:56:42Z
On Fri, Mar 23, 2018 at 4:43 AM, Peter Geoghegan <pg@bowt.ie> wrote: > On Thu, Mar 22, 2018 at 11:42 AM, Pavan Deolasee > <pavan.deolasee@gmail.com> wrote: > > A slightly improved version attached. > > You still need to remove this change: > > > diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h > > index a4574cd533..dbfb5d2a1a 100644 > > --- a/src/include/miscadmin.h > > +++ b/src/include/miscadmin.h > > @@ -444,5 +444,6 @@ extern bool has_rolreplication(Oid roleid); > > /* in access/transam/xlog.c */ > > extern bool BackupInProgress(void); > > extern void CancelBackup(void); > > +extern int64 GetXactWALBytes(void); > Sigh. Fixed in the recent version. > > I see that we're still using two target RTEs in this latest revision, > v23e -- the new approach to partitioning, which I haven't had time to > study in more detail, has not produced a change there. Yes, we continue to use two RTEs because I don't have any brighter idea than that to handle the case of partitioned table and right outer join. As I explained sometime back, this is necessary to ensure that we don't produce duplicate rows when a partition is joined with the source and then a second partition is joined again with the source. Now I don't know if we can run a join query and still have a single RTE, but that looks improbable and wrong. This causes > weird effects, such as the following: > > """ > pg@~[20658]=# create table foo(bar int4); > CREATE TABLE > pg@~[20658]=# merge into foo f using (select 1 col) dd on f.bar=dd.col > when matched then update set bar = f.barr + 1; > ERROR: column f.barr does not exist > LINE 1: ...n f.bar=dd.col when matched then update set bar = f.barr + 1... > ^ > HINT: Perhaps you meant to reference the column "f.bar" or the column > "f.bar". > > """ > > While I think this this particular HINT buglet is pretty harmless, I > continue to be concerned about the unintended consequences of having > multiple RTEs for MERGE's target table. Each RTE comes from a > different lookup path -- the first one goes through setTargetTable()'s > parserOpenTable() + addRangeTableEntryForRelation() calls. The second > one goes through transformFromClauseItem(), for the join, which > ultimately ends up calling transformTableEntry()/addRangeTableEntry(). > How's it different than running a INSERT query with the target table again specified in a subquery producing the rows to be inserted? For example, postgres=# insert into target as t select sid from source s join target t on t.ttid = s.sid; ERROR: column t.ttid does not exist LINE 1: ...rget as t select sid from source join target t on t.ttid = s... ^ HINT: Perhaps you meant to reference the column "t.tid" or the column "t.tid". postgres=# This produces a very similar looking HINT as your test above. I am certain that "target" table gets two RTEs, exactly via the same code paths as you discussed above. So if this is not a problem for INSERT, why it would be a problem for MERGE? May be I am missing a point here. > Consider commit 5f173040, which fixed a privilege escalation security > bug around multiple name lookup. Could the approach taken by MERGE > here introduce a similar security issue? > > Using GDB, I see two calls to RangeVarGetRelidExtended() when a simple > MERGE is executed. They both have identical relation arguments, that > look like this: > > (gdb) p *relation > $4 = { > type = T_RangeVar, > catalogname = 0x0, > schemaname = 0x0, > relname = 0x5600ebdcafb0 "foo", > inh = 1 '\001', > relpersistence = 112 'p', > alias = 0x5600ebdcb048, > location = 11 > } > > This seems like something that needs to be explained, at a minimum. > Even if I'm completely wrong about there being a security hazard, > maybe the suggestion that there might be still gives you some idea of > what I mean about unintended consequences. > Ok. I will try to explain it better and also think about the security hazards. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-03-24T06:15:01Z
On Fri, Mar 23, 2018 at 4:56 AM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > postgres=# insert into target as t select sid from source s join target t on > t.ttid = s.sid; > ERROR: column t.ttid does not exist > LINE 1: ...rget as t select sid from source join target t on t.ttid = s... > ^ > HINT: Perhaps you meant to reference the column "t.tid" or the column > "t.tid". > postgres=# > > This produces a very similar looking HINT as your test above. I am certain > that "target" table gets two RTEs, exactly via the same code paths as you > discussed above. So if this is not a problem for INSERT, why it would be a > problem for MERGE? May be I am missing a point here. I agree that this is very similar, as far as the RTEs go. What is dissimilar is the fact that there is hard-coded knowledge of both through parsing, planning, and execution. It's everything, taken together. ResultRelInfo has a ri_mergeTargetRTI field, which seems to be used instead of ri_RangeTableIndex in some contexts but not others. What might the interactions with something like GetInsertedColumns() and GetUpdatedColumns() be? Is that explained anywhere? In general, I think that there is potential for things to break in subtle ways. >> This seems like something that needs to be explained, at a minimum. >> Even if I'm completely wrong about there being a security hazard, >> maybe the suggestion that there might be still gives you some idea of >> what I mean about unintended consequences. > > > Ok. I will try to explain it better and also think about the security > hazards. I realize that I'm giving you a somewhat vague problem, without offering any real help on a solution. For what it's worth, I don't feel great about that, but I don't know enough about partitioning in general and your approach to partitioning for MERGE in particular to be more constructive. That said, checking that an issue like the one fixed by 5f173040 cannot recur here is one concrete thing you could do. Documenting/explaining the ri_RangeTableIndex/ri_mergeTargetRTI divide is another. The comment above ri_mergeTargetRTI is totally inadequate. -- Peter Geoghegan
-
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-03-24T06:35:01Z
On Fri, Mar 23, 2018 at 11:15 PM, Peter Geoghegan <pg@bowt.ie> wrote: > I agree that this is very similar, as far as the RTEs go. What is > dissimilar is the fact that there is hard-coded knowledge of both > through parsing, planning, and execution. It's everything, taken > together. > > ResultRelInfo has a ri_mergeTargetRTI field, which seems to be used > instead of ri_RangeTableIndex in some contexts but not others. What > might the interactions with something like GetInsertedColumns() and > GetUpdatedColumns() be? Is that explained anywhere? In general, I > think that there is potential for things to break in subtle ways. I just realized that there were no tests added to privileges.sql. You only have a small number of GRANT tests in merge.sql, for relation-level privileges, not column-level privileges. IOW, this area is totally untested. -- Peter Geoghegan
-
Re: [HACKERS] MERGE SQL Statement for PG11
Robert Haas <robertmhaas@gmail.com> — 2018-03-24T12:16:55Z
On Thu, Mar 22, 2018 at 7:13 PM, Peter Geoghegan <pg@bowt.ie> wrote: > While I think this this particular HINT buglet is pretty harmless, I > continue to be concerned about the unintended consequences of having > multiple RTEs for MERGE's target table. Each RTE comes from a > different lookup path -- the first one goes through setTargetTable()'s > parserOpenTable() + addRangeTableEntryForRelation() calls. The second > one goes through transformFromClauseItem(), for the join, which > ultimately ends up calling transformTableEntry()/addRangeTableEntry(). > Consider commit 5f173040, which fixed a privilege escalation security > bug around multiple name lookup. Could the approach taken by MERGE > here introduce a similar security issue? Yeah, that seems really bad. I don't think there is a huge problem with having multiple RTEs; for example, we very commonly end up with both rte->inh and !rte->inh RTEs for the same table, and that is OK. However, generating those RTEs by doing multiple name lookups for the same table is a big problem. Imagine, for example, that a user has a search_path of a, b and that there is a table b.foo. The user does a merge on foo. Between the first name lookup and the second, somebody creates a.foo. Now, potentially, half of the MERGE statement is going to be running against b.foo and the other half against a.foo. I don't know whether that will crash or bomb out with a strange error or just make some unexpected modification to one of those tables, but the behavior, even if not insecure, will certainly be wrong. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [HACKERS] MERGE SQL Statement for PG11
Robert Haas <robertmhaas@gmail.com> — 2018-03-24T12:27:36Z
On Sat, Mar 24, 2018 at 8:16 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Mar 22, 2018 at 7:13 PM, Peter Geoghegan <pg@bowt.ie> wrote: >> While I think this this particular HINT buglet is pretty harmless, I >> continue to be concerned about the unintended consequences of having >> multiple RTEs for MERGE's target table. Each RTE comes from a >> different lookup path -- the first one goes through setTargetTable()'s >> parserOpenTable() + addRangeTableEntryForRelation() calls. The second >> one goes through transformFromClauseItem(), for the join, which >> ultimately ends up calling transformTableEntry()/addRangeTableEntry(). >> Consider commit 5f173040, which fixed a privilege escalation security >> bug around multiple name lookup. Could the approach taken by MERGE >> here introduce a similar security issue? > > Yeah, that seems really bad. I don't think there is a huge problem > with having multiple RTEs; for example, we very commonly end up with > both rte->inh and !rte->inh RTEs for the same table, and that is OK. > However, generating those RTEs by doing multiple name lookups for the > same table is a big problem. Imagine, for example, that a user has a > search_path of a, b and that there is a table b.foo. The user does a > merge on foo. Between the first name lookup and the second, somebody > creates a.foo. Now, potentially, half of the MERGE statement is going > to be running against b.foo and the other half against a.foo. I don't > know whether that will crash or bomb out with a strange error or just > make some unexpected modification to one of those tables, but the > behavior, even if not insecure, will certainly be wrong. If it's possible to identify the two OIDs that are supposed to match and cross-check that the OIDs are the same, then we could just bomb out with an error if they aren't. That's not lovely, and is basically a hack, but it's possible that no better fix is possible in the time we have, and it's wouldn't be any worse than this crock from copy.c: if (!list_member_oid(plan->relationOids, queryRelId)) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("relation referenced by COPY statement has changed"))); Uggh, that code makes me hold my nose every time I look at it. But it's a cheap fix. (Hmm... I wonder if that's really an adequate fix for the problem in COPY, if we can't verify that the OID in question plays the right role in the query, rather than just that it's there somewhere. Anyway, if we can reliably identify the two OIDs to be compared, that's certainly good enough.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-24T12:28:26Z
On 24 March 2018 at 12:16, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Mar 22, 2018 at 7:13 PM, Peter Geoghegan <pg@bowt.ie> wrote: >> While I think this this particular HINT buglet is pretty harmless, I >> continue to be concerned about the unintended consequences of having >> multiple RTEs for MERGE's target table. Each RTE comes from a >> different lookup path -- the first one goes through setTargetTable()'s >> parserOpenTable() + addRangeTableEntryForRelation() calls. The second >> one goes through transformFromClauseItem(), for the join, which >> ultimately ends up calling transformTableEntry()/addRangeTableEntry(). >> Consider commit 5f173040, which fixed a privilege escalation security >> bug around multiple name lookup. Could the approach taken by MERGE >> here introduce a similar security issue? > > Yeah, that seems really bad. I don't think there is a huge problem > with having multiple RTEs; for example, we very commonly end up with > both rte->inh and !rte->inh RTEs for the same table, and that is OK. > However, generating those RTEs by doing multiple name lookups for the > same table is a big problem. Imagine, for example, that a user has a > search_path of a, b and that there is a table b.foo. The user does a > merge on foo. Between the first name lookup and the second, somebody > creates a.foo. Now, potentially, half of the MERGE statement is going > to be running against b.foo and the other half against a.foo. I don't > know whether that will crash or bomb out with a strange error or just > make some unexpected modification to one of those tables, but the > behavior, even if not insecure, will certainly be wrong. MERGE uses multiple RTEs in exactly the same way UPDATE does. I don't see a reason for specific concern wrt to MERGE. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-03-24T17:48:46Z
On Sat, Mar 24, 2018 at 5:27 AM, Robert Haas <robertmhaas@gmail.com> wrote: > If it's possible to identify the two OIDs that are supposed to match > and cross-check that the OIDs are the same, then we could just bomb > out with an error if they aren't. That's not lovely, and is basically > a hack, but it's possible that no better fix is possible in the time > we have, and it's wouldn't be any worse than this crock from copy.c: > > if (!list_member_oid(plan->relationOids, queryRelId)) > ereport(ERROR, > (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > errmsg("relation referenced by COPY statement > has changed"))); That's definitely all we have time for. The only alternative is to rip out support for partitioning, as partitioning is the only thing that necessitates the use of multiple RTEs. I don't think it would make sense to use a second RTE only when needed. -- Peter Geoghegan -
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-26T09:53:42Z
On 24 March 2018 at 12:27, Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, Mar 24, 2018 at 8:16 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Thu, Mar 22, 2018 at 7:13 PM, Peter Geoghegan <pg@bowt.ie> wrote: >>> While I think this this particular HINT buglet is pretty harmless, I >>> continue to be concerned about the unintended consequences of having >>> multiple RTEs for MERGE's target table. Each RTE comes from a >>> different lookup path -- the first one goes through setTargetTable()'s >>> parserOpenTable() + addRangeTableEntryForRelation() calls. The second >>> one goes through transformFromClauseItem(), for the join, which >>> ultimately ends up calling transformTableEntry()/addRangeTableEntry(). >>> Consider commit 5f173040, which fixed a privilege escalation security >>> bug around multiple name lookup. Could the approach taken by MERGE >>> here introduce a similar security issue? >> >> Yeah, that seems really bad. I don't think there is a huge problem >> with having multiple RTEs; for example, we very commonly end up with >> both rte->inh and !rte->inh RTEs for the same table, and that is OK. >> However, generating those RTEs by doing multiple name lookups for the >> same table is a big problem. Imagine, for example, that a user has a >> search_path of a, b and that there is a table b.foo. The user does a >> merge on foo. Between the first name lookup and the second, somebody >> creates a.foo. Now, potentially, half of the MERGE statement is going >> to be running against b.foo and the other half against a.foo. I don't >> know whether that will crash or bomb out with a strange error or just >> make some unexpected modification to one of those tables, but the >> behavior, even if not insecure, will certainly be wrong. > > If it's possible to identify the two OIDs that are supposed to match > and cross-check that the OIDs are the same, then we could just bomb > out with an error if they aren't. Since we now have MVCC catalog scans, all the name lookups are performed using the same snapshot so in the above scenario the newly created object would be invisible to the second name lookup. So I don't see anyway for the ERROR to occur and hence no need for a cross check, for UPDATE or MERGE. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-03-26T14:39:38Z
On Fri, Mar 23, 2018 at 4:37 PM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > > > On Fri, Mar 23, 2018 at 12:57 PM, Amit Langote < > Langote_Amit_f8@lab.ntt.co.jp> wrote: > >> >> Also, it seems that the delta patch I sent in the last email didn't >> contain all the changes I had to make. It didn't contain, for example, >> replacing adjust_and_expand_inherited_tlist() with >> adjust_partition_tlist(). I guess you'll know when you rebase anyway. >> > > Yes, I am planning to fix that once the ON CONFLICT patch is > ready/committed. > > Now that ON CONFLICT patch is in, here are rebased patches. The second patch is to add support for CTE (thanks Peter). Apart from rebase, the following things are fixed/improved: - Added test cases for column level privileges as suggested by Peter. One problem got discovered during the process. Since we expand and track source relation targetlist, the exiting code was demanding SELECT privileges on all attributes, even though MERGE is only referencing a few attributes on which the user has privilege. Fixed that by disassociating expansion from the actual referencing. - Added a test case for RLS where SELECT policy actually hides some rows, as suggested by Stephen in the past - Added check to compare result relation's and merge target relation's OIDs, as suggested by Robert. Simon thinks it's not necessary given that we now scan catalog using MVCC snapshot. So will leave it to his discretion when he takes it up for commit - Improved explanation regarding why we need a second RTE for merge target relation and general cleanup/improvements in that area I think it will be a good idea to send any further patches as add-on patches for reviewer/committer's sake. I will do that unless someone disagrees. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-26T15:05:26Z
On 26 March 2018 at 15:39, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > Now that ON CONFLICT patch is in, here are rebased patches. The second patch > is to add support for CTE (thanks Peter). > > Apart from rebase, the following things are fixed/improved: > > - Added test cases for column level privileges as suggested by Peter. One > problem got discovered during the process. Since we expand and track source > relation targetlist, the exiting code was demanding SELECT privileges on all > attributes, even though MERGE is only referencing a few attributes on which > the user has privilege. Fixed that by disassociating expansion from the > actual referencing. Good catch Peter. > - Added a test case for RLS where SELECT policy actually hides some rows, as > suggested by Stephen in the past > > - Added check to compare result relation's and merge target relation's OIDs, > as suggested by Robert. Simon thinks it's not necessary given that we now > scan catalog using MVCC snapshot. So will leave it to his discretion when he > takes it up for commit No problem with adding the test, its quick to compare two Oids. > - Improved explanation regarding why we need a second RTE for merge target > relation and general cleanup/improvements in that area > I think it will be a good idea to send any further patches as add-on patches > for reviewer/committer's sake. I will do that unless someone disagrees. +1 So v25 is the "commit candidate" and we can add other patches to it. Given recent bugfix/changes I don't plan to commit this tomorrow anymore. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Robert Haas <robertmhaas@gmail.com> — 2018-03-26T15:09:42Z
On Mon, Mar 26, 2018 at 5:53 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > Since we now have MVCC catalog scans, all the name lookups are > performed using the same snapshot so in the above scenario the newly > created object would be invisible to the second name lookup. That's not true, because each lookup would be performed using a new snapshot -- not all under one snapshot. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-26T16:06:10Z
On 26 March 2018 at 15:39, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > reviewer 1. In ExecMergeMatched() we have a line of code that does this... if (TransactionIdIsCurrentTransactionId(hufd.xmax)) then errcode(ERRCODE_CARDINALITY_VIOLATION) I notice this is correct, but too strong. It should be possible to run a sequence of MERGE commands inside a transaction, repeatedly updating the same set of rows, as is possible with UPDATE. We need to check whether the xid is the current subxid and the cid is the current commandid, rather than using TransactionIdIsCurrentTransactionId() On further analysis, I note that ON CONFLICT suffers from this problem as well, looks like I just refactored it from there. 2. EXPLAIN ANALYZE looks unchanged from some time back. The math is only correct as long as there are zero rows that do not cause an INS/UPD/DEL. We don't test for that. I think this is a regression from an earlier report of the same bug, or perhaps I didn't fix it at the time. 3. sp. depedning trigger.sgml 4. trigger.sgml replace "specific actions specified" with "events specified in actions" to avoid the double use of "specific" 5. I take it the special code for TransformMerge target relations is replaced by "right_rte"? Seems fragile to leave it like that. Can we add an Assert()? Do we care? 6. I didn't understand "Assume that the top-level join RTE is at the end. The source relation + * is just before that." What is there is no source relation? 7. The formatting of the SQL statement in transformMergeStmt that begins "Construct a query of the form" is borked, so the SQL layout is unclear, just needs pretty print 8. I didn't document why I thought this was true "XXX if we have a constant subquery, we can also skip join", but one of the explain analyze outputs shows this is already true - where we provide a constant query and it skips the join. So perhaps we can remove the comment. (Search for "Seq Scan on target t_1") 9. I think we need to mention that MERGE won't work with rules or inheritance (only partitioning) in the main doc page. The current text says that rules are ignored, which would be true if we didn't specifically throw ERROR feature not supported. 10. Comment needs updating for changes in code below it - "In MERGE when and condition, no system column is allowed" 11. In comment "Since the plan re-evaluated by EvalPlanQual uses the second RTE", suggest using "join RTE" to make it more explicit which RTE we are discussing 12. Missed out merge.sgml from v25 patch. 13. For triggers we say "No separate triggers are defined for <command>MERGE</command>" we should also state the same caveat for POLICY events That's all I can see so far. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-26T16:16:37Z
On 26 March 2018 at 16:09, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Mar 26, 2018 at 5:53 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> Since we now have MVCC catalog scans, all the name lookups are >> performed using the same snapshot so in the above scenario the newly >> created object would be invisible to the second name lookup. > > That's not true, because each lookup would be performed using a new > snapshot -- not all under one snapshot. You're saying we take a separate snapshot for each table we lookup? Sounds weird to me. So this error could happen in SELECT, UPDATE, DELETE or INSERT as well. Or you see this as something related specifically to MERGE, if so how? Please explain what you see. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Robert Haas <robertmhaas@gmail.com> — 2018-03-26T16:52:37Z
On Mon, Mar 26, 2018 at 12:16 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 26 March 2018 at 16:09, Robert Haas <robertmhaas@gmail.com> wrote: >> On Mon, Mar 26, 2018 at 5:53 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>> Since we now have MVCC catalog scans, all the name lookups are >>> performed using the same snapshot so in the above scenario the newly >>> created object would be invisible to the second name lookup. >> >> That's not true, because each lookup would be performed using a new >> snapshot -- not all under one snapshot. > > You're saying we take a separate snapshot for each table we lookup? > Sounds weird to me. I'm saying we take a separate snapshot for each and every catalog lookup, except when we know that no catalog changes can have occurred. See the commit message for 568d4138c646cd7cd8a837ac244ef2caf27c6bb8. If you do a lookup in pg_class and 3 lookups in pg_attribute each of the 4 can be done under a different snapshot, in the worst case. You're not the first person to believe that the MVCC catalog scan patch fixes that problem, but as the guy who wrote it, it definitely doesn't. What that patch fixed was, prior to that patch, a catalog scan might find the WRONG NUMBER OF ROWS, like you might do a lookup against a unique index for an object that existed and, if the row was concurrently updated, you might find 0 rows or 2 rows instead of 1 row. IOW, it guaranteed that we used a consistent snapshot for each individual lookup, not a consistent snapshot for the whole course of a command. > So this error could happen in SELECT, UPDATE, DELETE or INSERT as well. > > Or you see this as something related specifically to MERGE, if so how? > Please explain what you see. As I said before, the problem occurs if the same command looks up the same table name in more than one place. There is absolutely nothing to guarantee that we get the same answer every time. As far as I know, the proposed MERGE patch has that issue an existing DML commands don't; but someone else may have better information. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-26T19:17:03Z
On 26 March 2018 at 17:52, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Mar 26, 2018 at 12:16 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> On 26 March 2018 at 16:09, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Mon, Mar 26, 2018 at 5:53 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>>> Since we now have MVCC catalog scans, all the name lookups are >>>> performed using the same snapshot so in the above scenario the newly >>>> created object would be invisible to the second name lookup. >>> >>> That's not true, because each lookup would be performed using a new >>> snapshot -- not all under one snapshot. >> >> You're saying we take a separate snapshot for each table we lookup? >> Sounds weird to me. > > I'm saying we take a separate snapshot for each and every catalog > lookup, except when we know that no catalog changes can have occurred. > See the commit message for 568d4138c646cd7cd8a837ac244ef2caf27c6bb8. > If you do a lookup in pg_class and 3 lookups in pg_attribute each of > the 4 can be done under a different snapshot, in the worst case. > You're not the first person to believe that the MVCC catalog scan > patch fixes that problem, but as the guy who wrote it, it definitely > doesn't. What that patch fixed was, prior to that patch, a catalog > scan might find the WRONG NUMBER OF ROWS, like you might do a lookup > against a unique index for an object that existed and, if the row was > concurrently updated, you might find 0 rows or 2 rows instead of 1 > row. IOW, it guaranteed that we used a consistent snapshot for each > individual lookup, not a consistent snapshot for the whole course of a > command. That all makes sense, thanks for explaining. I spent a few more minutes, going "but", "but" though I can now see good reasons for everything to work this way. >> So this error could happen in SELECT, UPDATE, DELETE or INSERT as well. >> >> Or you see this as something related specifically to MERGE, if so how? >> Please explain what you see. > > As I said before, the problem occurs if the same command looks up the > same table name in more than one place. There is absolutely nothing > to guarantee that we get the same answer every time. > As far as I > know, the proposed MERGE patch has that issue an existing DML commands > don't; but someone else may have better information. I will look deeper and report back. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-03-26T22:10:58Z
On Mon, Mar 26, 2018 at 12:17 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> As far as I >> know, the proposed MERGE patch has that issue an existing DML commands >> don't; but someone else may have better information. > > I will look deeper and report back. It's quite clear that the problem exists with the MERGE patch; the simple fact that RangeVarGetRelidExtended() is called twice with the same RangeVar argument shows this. However, the Oid cross-check seems like a sufficient defense against an inconsistency that causes real trouble, since the cross-check will only error-out when a concurrent table creation (or maybe ALTER TABLE) makes a second table visible, in a schema that appears earlier in the user's search_path. It's hard to imagine any legitimate user truly preferring some alternative behavior in this particular scenario, which makes it okay. This cross-check workaround is ugly, but apparently there is a precedent in copy.c. I didn't know that detail until Robert pointed it out. That makes me feel a lot better about this general question of how the target relation is represented, having two RTEs, etc. -- Peter Geoghegan
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-27T08:15:20Z
On 26 March 2018 at 23:10, Peter Geoghegan <pg@bowt.ie> wrote: > On Mon, Mar 26, 2018 at 12:17 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >>> As far as I >>> know, the proposed MERGE patch has that issue an existing DML commands >>> don't; but someone else may have better information. >> >> I will look deeper and report back. > > It's quite clear that the problem exists with the MERGE patch Accepted, the only question is whether it affects UPDATE as well cos it looks like it should. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-27T08:24:49Z
On 26 March 2018 at 17:06, Simon Riggs <simon@2ndquadrant.com> wrote: > On 26 March 2018 at 15:39, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > > That's all I can see so far. * change comment “once to” to “once” in src/include/nodes/execnodes.h * change comment “and to run” to “and once to run” * change “result relation” to “target relation” * XXX we probably need to check plan output for CMD_MERGE also * Spurious line feed in src/backend/optimizer/prep/preptlist.c * No need to remove whitespace in src/backend/optimizer/util/relnode.c * README should note that the TABLEOID junk column is not strictly needed when joining to a non-partitioned table but we don't try to optimize that away. Is that an XXX to fix in future or do we just think the extra 4 bytes won't make much difference so we leave it? * Comment in rewriteTargetListMerge() should mention TABLEOID exists to allow us to find the correct relation, not the correct row, comment just copied from CTID above it. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-03-27T09:28:36Z
On Mon, Mar 26, 2018 at 9:36 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 26 March 2018 at 15:39, Pavan Deolasee <pavan.deolasee@gmail.com> > wrote: > > > reviewer > > 1. In ExecMergeMatched() we have a line of code that does this... > > if (TransactionIdIsCurrentTransactionId(hufd.xmax)) > then errcode(ERRCODE_CARDINALITY_VIOLATION) > > I notice this is correct, but too strong. It should be possible to run > a sequence of MERGE commands inside a transaction, repeatedly updating > the same set of rows, as is possible with UPDATE. > > We need to check whether the xid is the current subxid and the cid is > the current commandid, rather than using > TransactionIdIsCurrentTransactionId() > > AFAICS this is fine because we invoke that code only when HeapTupleSatisfiesUpdate returns HeapTupleSelfUpdated i.e. for the case when the tuple is updated by our transaction after the scan is started. HeapTupleSatisfiesUpdate already checks for command id before returning HeapTupleSelfUpdated. > > 2. EXPLAIN ANALYZE looks unchanged from some time back. The math is > only correct as long as there are zero rows that do not cause an > INS/UPD/DEL. > We don't test for that. I think this is a regression from an earlier > report of the same bug, or perhaps I didn't fix it at the time. > I've now added a separate counter to count all three actions and we also report "Tuples skipped" which could be either because there was no action to handle that source tuple or quals did not match. Added regression tests specific to EXPLAIN ANALYZE. > > 3. sp. depedning trigger.sgml > Fixed. > > 4. trigger.sgml replace "specific actions specified" with "events > specified in actions" > to avoid the double use of "specific" > Fixed. > > 5. I take it the special code for TransformMerge target relations is > replaced by "right_rte"? Seems fragile to leave it like that. Can we > add an Assert()? Do we care? > I didn't get this point. Can you please explain? > > 6. I didn't understand "Assume that the top-level join RTE is at the > end. The source relation > + * is just before that." > What is there is no source relation? > Can that happen? I mean shouldn't there always be a source relation? It could be a subquery or a function scan or just a plain relation, but something, right? > > 7. The formatting of the SQL statement in transformMergeStmt that > begins "Construct a query of the form" is borked, so the SQL layout is > unclear, just needs pretty print > Fixed. > > 8. I didn't document why I thought this was true "XXX if we have a > constant subquery, we can also skip join", but one of the explain > analyze outputs shows this is already true - where we provide a > constant query and it skips the join. So perhaps we can remove the > comment. (Search for "Seq Scan on target t_1") > Agree, removed. > > 9. I think we need to mention that MERGE won't work with rules or > inheritance (only partitioning) in the main doc page. The current > text says that rules are ignored, which would be true if we didn't > specifically throw ERROR feature not supported. > > Added a short para to merge.sgml > 10. Comment needs updating for changes in code below it - "In MERGE > when and condition, no system column is allowed" > > Yeah, that's kinda half-true since the code below supports TABLEOID and OID system columns. I am thinking about this in a larger context though. Peter has expressed desire to support system columns in WHEN targetlist and quals. I gave it a try and it seems if we remove that error block, all system columns are supported readily. But only from the target side. There is a problem if we try to refer a system column from the source side since the mergrSourceTargetList only includes user columns and so set_plan_refs() complains about a system column. I am not sure what's the best way to handle this. May be we can add system columns to the mergrSourceTargetList. I haven't yet found a neat way to do that. > 11. In comment "Since the plan re-evaluated by EvalPlanQual uses the > second RTE", suggest using "join RTE" to make it more explicit which > RTE we are discussing > Ok, fixed. > > 12. Missed out merge.sgml from v25 patch. > Ouch, added. Also generating a new patch which includes merge.sgml and sending other improvements as add-ons. > > 13. For triggers we say "No separate triggers are defined for > <command>MERGE</command>" > we should also state the same caveat for POLICY events > Ok. Added a short para in create_policy.sgml Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-03-27T09:31:33Z
On Tue, Mar 27, 2018 at 1:54 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 26 March 2018 at 17:06, Simon Riggs <simon@2ndquadrant.com> wrote: > > On 26 March 2018 at 15:39, Pavan Deolasee <pavan.deolasee@gmail.com> > wrote: > > > > > That's all I can see so far. > > * change comment “once to” to “once” in src/include/nodes/execnodes.h > * change comment “and to run” to “and once to run” > * change “result relation” to “target relation” > Fixed all of that in the patch v26 set I just sent. > > * XXX we probably need to check plan output for CMD_MERGE also > Yeah. Added those checks for MERGE action's target lists in v26. > > * Spurious line feed in src/backend/optimizer/prep/preptlist.c > Couldn't spot it. Will look closer, but any hint will be appreciated. > > * No need to remove whitespace in src/backend/optimizer/util/relnode.c > Fixed in v26. > > * README should note that the TABLEOID junk column is not strictly > needed when joining to a non-partitioned table but we don't try to > optimize that away. Is that an XXX to fix in future or do we just > think the extra 4 bytes won't make much difference so we leave it? > I actually took the opportunity to conditionally fetch tableoid only if we are dealing with partitioned table. > > * Comment in rewriteTargetListMerge() should mention TABLEOID exists > to allow us to find the correct relation, not the correct row, comment > just copied from CTID above it. > > Fixed in v26. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-27T09:40:35Z
On 27 March 2018 at 10:28, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: >> 1. In ExecMergeMatched() we have a line of code that does this... >> >> if (TransactionIdIsCurrentTransactionId(hufd.xmax)) >> then errcode(ERRCODE_CARDINALITY_VIOLATION) >> >> I notice this is correct, but too strong. It should be possible to run >> a sequence of MERGE commands inside a transaction, repeatedly updating >> the same set of rows, as is possible with UPDATE. >> >> We need to check whether the xid is the current subxid and the cid is >> the current commandid, rather than using >> TransactionIdIsCurrentTransactionId() > > AFAICS this is fine because we invoke that code only when > HeapTupleSatisfiesUpdate returns HeapTupleSelfUpdated i.e. for the case when > the tuple is updated by our transaction after the scan is started. > HeapTupleSatisfiesUpdate already checks for command id before returning > HeapTupleSelfUpdated. Cool. >> 5. I take it the special code for TransformMerge target relations is >> replaced by "right_rte"? Seems fragile to leave it like that. Can we >> add an Assert()? Do we care? > > I didn't get this point. Can you please explain? The code confused me at that point. More docs pls. >> 6. I didn't understand "Assume that the top-level join RTE is at the >> end. The source relation >> + * is just before that." >> What is there is no source relation? > > > Can that happen? I mean shouldn't there always be a source relation? It > could be a subquery or a function scan or just a plain relation, but > something, right? Yes, OK. So ordering is target, source, join. >> 10. Comment needs updating for changes in code below it - "In MERGE >> when and condition, no system column is allowed" >> > > Yeah, that's kinda half-true since the code below supports TABLEOID and OID > system columns. I am thinking about this in a larger context though. Peter > has expressed desire to support system columns in WHEN targetlist and quals. > I gave it a try and it seems if we remove that error block, all system > columns are supported readily. But only from the target side. There is a > problem if we try to refer a system column from the source side since the > mergrSourceTargetList only includes user columns and so set_plan_refs() > complains about a system column. > > I am not sure what's the best way to handle this. May be we can add system > columns to the mergrSourceTargetList. I haven't yet found a neat way to do > that. I was saying the comment needs changing, not the code. Cool, thanks -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-27T10:46:55Z
On 27 March 2018 at 10:31, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > Fixed in v26. More comments on v26 * Change errmsg “Ensure that not more than one source rows match any one target row” should be “Ensure that not more than one source row matches any one target row” * I think we need an example in the docs showing a constant source query, so people can understand how to use MERGE for OLTP as well as large ELT * Long comment in ExecMerge() needs rewording, formatting and spell check I suggest not referring to an "order" since that concept doesn't exist anywhere else * Need tests for coverage of these ERROR messages Named security policy violation SELECT not allowed in MERGE INSERT... Multiple VALUES clauses not... MERGE is not supported for this... MERGE is not supported for relations with inheritance MERGE is not supported for relations with rules -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-27T11:30:54Z
On 27 March 2018 at 11:46, Simon Riggs <simon@2ndquadrant.com> wrote: > On 27 March 2018 at 10:31, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > >> Fixed in v26. > > More comments on v26 In terms of further performance optimization, if there is just one WHEN AND condition and no unconditional WHEN clauses then we can add the WHEN AND easily to the join query. That seems like an easy thing to do for PG11 -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-03-27T18:14:58Z
On Tue, Mar 27, 2018 at 1:15 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > Accepted, the only question is whether it affects UPDATE as well cos > it looks like it should. If you mean an UPDATE FROM self-join, then I suppose that it does, in a very limited way. The difference is that there are no hard-coded assumptions about the relationship between those two RTEs. -- Peter Geoghegan
-
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-03-28T02:58:21Z
On Tue, Mar 27, 2018 at 2:28 AM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > (Version 26) I have some feedback on this version: * ExecMergeMatched() needs to determine tuple lock mode for EvalPlanQual() in a way that's based on how everything else works; it's not okay to just use LockTupleExclusive in all cases. That will often lead to lock escalation, which can cause unprincipled deadlocks. You need to pass back the relevant info from routines like heap_update(), which means more state needs to come back to ExecMergeMatched() from routines like ExecUpdate(). * Doesn't ExecUpdateLockMode(), which is called from places like ExecBRUpdateTriggers(), also need to be taught about GetEPQRangeTableIndex() (i.e. the ri_mergeTargetRTI/ri_RangeTableIndex divide)? You should audit everything like that carefully. Maybe GetEPQRangeTableIndex() is not the best choke-point to do this kind of thing. Not that I have a clearly better idea. * Looks like there is a similar problem in ExecPartitionCheckEmitError(). I don't really understand how that works, so I might be wrong here. * More or less the same issue seems to exist within ExecConstraints(), including where GetInsertedColumns() is used. * Compiler warning: fwrapv -fexcess-precision=standard -Og -g3 -fno-omit-frame-pointer -I../../../src/include -I/code/postgresql/root/build/../source/src/include -D_FORTIFY_SOURCE=2 -D TRUST_STRXFRM -D LOCK_DEBUG -D WAL_DEBUG -D BTREE_BUILD_STATS -D MULTIXACT_DEBUG -D SELECTIVITY_DEBUG -D HJDEBUG -D_GNU_SOURCE -I/usr/include/libxml2 -c -o nodeModifyTable.o /code/postgresql/root/build/../source/src/backend/executor/nodeModifyTable.c /code/postgresql/root/build/../source/src/backend/executor/nodeModifyTable.c: In function ‘ExecInsert’: /code/postgresql/root/build/../source/src/backend/executor/nodeModifyTable.c:412:4: warning: ‘wco_kind’ may be used uninitialized in this function [-Wmaybe-uninitialized] ExecWithCheckOptions(wco_kind, resultRelInfo, slot, estate); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * The BufferIsValid() checks to decide if we need to ReleaseBuffer() within ExecMergeMatched() are unnecessary -- a buffer pin must be held throughout. This looks like it's leftover from before the ExecMergeNotMatched()/ExecMergeMatched() split was made. * There should be ResetExprContext() calls for your new MergeActionState projections. That's what we see for the RETURNING + ON CONFLICT projections within nodeModifyTable.c, which the new projections are very similar to, and clearly modeled on. -- Peter Geoghegan -
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-03-28T04:30:41Z
On Wed, Mar 28, 2018 at 8:28 AM, Peter Geoghegan <pg@bowt.ie> wrote: > On Tue, Mar 27, 2018 at 2:28 AM, Pavan Deolasee > <pavan.deolasee@gmail.com> wrote: > > (Version 26) > > I have some feedback on this version: > > * ExecMergeMatched() needs to determine tuple lock mode for > EvalPlanQual() in a way that's based on how everything else works; > it's not okay to just use LockTupleExclusive in all cases. That will > often lead to lock escalation, which can cause unprincipled deadlocks. > You need to pass back the relevant info from routines like > heap_update(), which means more state needs to come back to > ExecMergeMatched() from routines like ExecUpdate(). > You're right. I am thinking what would be a good way to pass that information back. Should we add a new out parameter to ExecUpdate() or a new member to HeapUpdateFailureData? It seems only ExecUpdate() would need the change, so may be it's fine to change the API but HeapUpdateFailureData doesn't look bad either since it deals with failure cases and we are indeed dealing with ExecUpdate() failure. Any preference? > > * Doesn't ExecUpdateLockMode(), which is called from places like > ExecBRUpdateTriggers(), also need to be taught about > GetEPQRangeTableIndex() (i.e. the ri_mergeTargetRTI/ri_RangeTableIndex > divide)? You should audit everything like that carefully. Maybe > GetEPQRangeTableIndex() is not the best choke-point to do this kind of > thing. Not that I have a clearly better idea. > > * Looks like there is a similar problem in > ExecPartitionCheckEmitError(). I don't really understand how that > works, so I might be wrong here. > > * More or less the same issue seems to exist within ExecConstraints(), > including where GetInsertedColumns() is used. > They all look fine to me. Remember that we always resolve column references in WHEN targetlist from the target relation and hence things like updatedCols/insertedCols are get set on the target RTE. All of these routines read from the target RTE as far as I can see. But I will check in more detail. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-03-28T11:00:42Z
On Wed, Mar 28, 2018 at 8:28 AM, Peter Geoghegan <pg@bowt.ie> wrote: > > * ExecMergeMatched() needs to determine tuple lock mode for > EvalPlanQual() in a way that's based on how everything else works; > it's not okay to just use LockTupleExclusive in all cases. That will > often lead to lock escalation, which can cause unprincipled deadlocks. > You need to pass back the relevant info from routines like > heap_update(), which means more state needs to come back to > ExecMergeMatched() from routines like ExecUpdate(). > Fixed by adding a new member to HeapUpdateFailureData. That seemed more natural, but we can change if others disagree. > > * Doesn't ExecUpdateLockMode(), which is called from places like > ExecBRUpdateTriggers(), also need to be taught about > GetEPQRangeTableIndex() (i.e. the ri_mergeTargetRTI/ri_RangeTableIndex > divide)? You should audit everything like that carefully. Maybe > GetEPQRangeTableIndex() is not the best choke-point to do this kind of > thing. Not that I have a clearly better idea. > > * Looks like there is a similar problem in > ExecPartitionCheckEmitError(). I don't really understand how that > works, so I might be wrong here. > > * More or less the same issue seems to exist within ExecConstraints(), > including where GetInsertedColumns() is used. > As I said, I do not see a problem with this. The insertedCols/updatedCols etc are tracked in the target table's RTE and hence we should continue to use ri_RangeTableIndex for such purposes. The ri_mergeTargetRTI is only useful to running EvalPlanQual correctly. > > * Compiler warning: > > fwrapv -fexcess-precision=standard -Og -g3 -fno-omit-frame-pointer > -I../../../src/include > -I/code/postgresql/root/build/../source/src/include > -D_FORTIFY_SOURCE=2 -D TRUST_STRXFRM -D LOCK_DEBUG -D WAL_DEBUG -D > BTREE_BUILD_STATS -D MULTIXACT_DEBUG -D SELECTIVITY_DEBUG -D HJDEBUG > -D_GNU_SOURCE -I/usr/include/libxml2 -c -o nodeModifyTable.o > /code/postgresql/root/build/../source/src/backend/executor/ > nodeModifyTable.c > /code/postgresql/root/build/../source/src/backend/executor/ > nodeModifyTable.c: > In function ‘ExecInsert’: > /code/postgresql/root/build/../source/src/backend/executor/ > nodeModifyTable.c:412:4: > warning: ‘wco_kind’ may be used uninitialized in this function > [-Wmaybe-uninitialized] > ExecWithCheckOptions(wco_kind, resultRelInfo, slot, estate); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > Fixed. > > * The BufferIsValid() checks to decide if we need to ReleaseBuffer() > within ExecMergeMatched() are unnecessary -- a buffer pin must be held > throughout. This looks like it's leftover from before the > ExecMergeNotMatched()/ExecMergeMatched() split was made. > Fixed. > > * There should be ResetExprContext() calls for your new > MergeActionState projections. That's what we see for the RETURNING + > ON CONFLICT projections within nodeModifyTable.c, which the new > projections are very similar to, and clearly modeled on. > Right. I thought about what would the right place to call ResetExprContext() and chose to do that in ExecMerge(). I am actually a bit surprised that ExecProcessReturning() and ExecOnConflictUpdate() call ResetExprContext(). Aren't they both using mtstate->ps.ps_ExprContext? If so, why is it safe to reset the expression context before we are completely done with the previous tuple? Clearly, the current code is working fine, so may be there is no bug, but it looks curious. On Tue, Mar 27, 2018 at 4:16 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > > More comments on v26 > > * Change errmsg “Ensure that not more than one source rows match any > one target row” > should be > “Ensure that not more than one source row matches any one target row” > Fixed. > > * I think we need an example in the docs showing a constant source > query, so people can understand how to use MERGE for OLTP as well as > large ELT > > * Long comment in ExecMerge() needs rewording, formatting and spell check > Tried. Please check and suggest improvements, if any. > I suggest not referring to an "order" since that concept doesn't exist > anywhere else > You mean when we say that the actions are evaluated "in an order"? > > * Need tests for coverage of these ERROR messages > Named security policy violation > Surprisingly none exists even for regular UPDATE/INSERT/DELETE. I will see what is needed to trigger that error message. > SELECT not allowed in MERGE INSERT... > Multiple VALUES clauses not... > MERGE is not supported for this... > MERGE is not supported for relations with inheritance > MERGE is not supported for relations with rules Added a test for each of those. v27 attached, though review changes are in the add-on 0005 patch. Apart from that - ran the patch through spell-checker and fixed a few typos - reorganised the code in 0004 a bit. - rebased on current master Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-03-29T06:37:20Z
On Tue, Mar 27, 2018 at 5:00 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > > In terms of further performance optimization, if there is just one > WHEN AND condition and no unconditional WHEN clauses then we can add > the WHEN AND easily to the join query. > > That seems like an easy thing to do for PG11 > > I think we need to be careful in terms of what can be pushed down to the join, in presence of WHEN NOT MATCHED actions. If we push the WHEN AND qual to the join then I am worried that some rows which should have been reported "matched" and later filtered out as part of the WHEN quals, will get reported as "not-matched", thus triggering WHEN NOT MATCHED action. For example, postgres=# select * from target ; a | b ---+---- 1 | 10 2 | 20 (2 rows) postgres=# select * from source ; a | b ---+----- 2 | 200 3 | 300 (2 rows) postgres=# BEGIN; BEGIN postgres=# EXPLAIN ANALYZE MERGE INTO target t USING source s ON t.a = s.a WHEN MATCHED AND t.a < 2 THEN UPDATE SET b = s.b WHEN NOT MATCHED THEN INSERT VALUES (s.a, -1); QUERY PLAN --------------------------------------------------------------------------------------------------------------------------- Merge on target t (cost=317.01..711.38 rows=25538 width=46) (actual time=0.104..0.104 rows=0 loops=1) * Tuples Inserted: 1* Tuples Updated: 0 Tuples Deleted: 0 * Tuples Skipped: 1* -> Merge Left Join (cost=317.01..711.38 rows=25538 width=46) (actual time=0.071..0.074 rows=2 loops=1) Merge Cond: (s.a = t_1.a) -> Sort (cost=158.51..164.16 rows=2260 width=40) (actual time=0.042..0.043 rows=2 loops=1) Sort Key: s.a Sort Method: quicksort Memory: 25kB -> Seq Scan on source s (cost=0.00..32.60 rows=2260 width=40) (actual time=0.027..0.031 rows=2 loops=1) -> Sort (cost=158.51..164.16 rows=2260 width=10) (actual time=0.019..0.020 rows=2 loops=1) Sort Key: t_1.a Sort Method: quicksort Memory: 25kB -> Seq Scan on target t_1 (cost=0.00..32.60 rows=2260 width=10) (actual time=0.012..0.014 rows=2 loops=1) Planning Time: 0.207 ms Execution Time: 0.199 ms (17 rows) postgres=# abort; ROLLBACK postgres=# BEGIN; BEGIN postgres=# EXPLAIN ANALYZE MERGE INTO target t USING source s ON t.a = s.a AND t.a < 2 WHEN MATCHED THEN UPDATE SET b = s.b WHEN NOT MATCHED THEN INSERT VALUES (s.a, -1); QUERY PLAN -------------------------------------------------------------------------------------------------------------------------- Merge on target t (cost=232.74..364.14 rows=8509 width=46) (actual time=0.128..0.128 rows=0 loops=1) * Tuples Inserted: 2* Tuples Updated: 0 Tuples Deleted: 0 Tuples Skipped: 0 -> Merge Right Join (cost=232.74..364.14 rows=8509 width=46) (actual time=0.070..0.072 rows=2 loops=1) Merge Cond: (t_1.a = s.a) -> Sort (cost=74.23..76.11 rows=753 width=10) (actual time=0.038..0.039 rows=1 loops=1) Sort Key: t_1.a Sort Method: quicksort Memory: 25kB -> Seq Scan on target t_1 (cost=0.00..38.25 rows=753 width=10) (actual time=0.026..0.028 rows=1 loops=1) Filter: (a < 2) Rows Removed by Filter: 1 -> Sort (cost=158.51..164.16 rows=2260 width=40) (actual time=0.024..0.025 rows=2 loops=1) Sort Key: s.a Sort Method: quicksort Memory: 25kB -> Seq Scan on source s (cost=0.00..32.60 rows=2260 width=40) (actual time=0.014..0.017 rows=2 loops=1) Planning Time: 0.218 ms Execution Time: 0.234 ms (19 rows) postgres=# abort; ROLLBACK If you look at the first MERGE statement, we filter one matched source row (2,200) using (t.a < 2) and do not take any action for that row. This filtering happens after the RIGHT JOIN has reported it as "matched". But if we push down the qual to the join, then the join will see that the source row has no match and hence send that row for NOT MATCHED processing, thus inserting it into the table again. I am not saying there is no scope for improvement. But we need to be careful about what can be pushed down to the join and what must be applied after the join. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-29T06:56:38Z
On 29 March 2018 at 07:37, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > > > On Tue, Mar 27, 2018 at 5:00 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> >> >> In terms of further performance optimization, if there is just one >> WHEN AND condition and no unconditional WHEN clauses then we can add >> the WHEN AND easily to the join query. >> >> That seems like an easy thing to do for PG11 >> > > I think we need to be careful in terms of what can be pushed down to the > join, in presence of WHEN NOT MATCHED actions. If we push the WHEN AND qual > to the join then I am worried that some rows which should have been reported > "matched" and later filtered out as part of the WHEN quals, will get > reported as "not-matched", thus triggering WHEN NOT MATCHED action. > postgres=# EXPLAIN ANALYZE MERGE INTO target t USING source s ON t.a = s.a > WHEN MATCHED AND t.a < 2 THEN UPDATE SET b = s.b WHEN NOT MATCHED THEN > INSERT VALUES (s.a, -1); That has an unconditional WHEN clause, so would block the push down using my stated rule above. With something like this MERGE INTO target t USING source s ON t.a = s.a WHEN MATCHED AND t.a < 2 THEN UPDATE SET b = s.b; or this MERGE INTO target t USING source s ON t.a = s.a WHEN MATCHED AND t.a < 2 THEN UPDATE SET b = s.b WHEN NOT MATCHED DO NOTHING; or this MERGE INTO target t USING source s ON t.a = s.a WHEN MATCHED AND t.a < 2 THEN UPDATE SET b = s.b WHEN MATCHED DO NOTHING WHEN NOT MATCHED DO NOTHING; then we can push down "t.a < 2" into the WHERE clause of the join query. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-29T09:50:02Z
On 28 March 2018 at 12:00, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > v27 attached, though review changes are in > the add-on 0005 patch. This all looks good now, thanks for making all of those changes. I propose [v27 patch1+patch3+patch5] as the initial commit candidate for MERGE, with other patches following later before end CF. I propose to commit this tomorrow, 30 March, about 26 hours from now. That will allow some time for buildfarm fixing/reversion before the Easter weekend, then other patches to follow starting 2 April. That then gives reasonable time to follow up on other issues that we will no doubt discover fairly soon after commit, such as additional runs by SQLsmith and more eyeballs. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-03-30T11:10:04Z
On 29 March 2018 at 10:50, Simon Riggs <simon@2ndquadrant.com> wrote: > On 28 March 2018 at 12:00, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > >> v27 attached, though review changes are in >> the add-on 0005 patch. > > This all looks good now, thanks for making all of those changes. > > I propose [v27 patch1+patch3+patch5] as the initial commit candidate > for MERGE, with other patches following later before end CF. > > I propose to commit this tomorrow, 30 March, about 26 hours from now. > That will allow some time for buildfarm fixing/reversion before the > Easter weekend, then other patches to follow starting 2 April. That > then gives reasonable time to follow up on other issues that we will > no doubt discover fairly soon after commit, such as additional runs by > SQLsmith and more eyeballs. No problems found, but moving proposed commit to 2 April pm -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Andres Freund <andres@anarazel.de> — 2018-04-03T02:18:00Z
Hi, On 2018-03-30 12:10:04 +0100, Simon Riggs wrote: > On 29 March 2018 at 10:50, Simon Riggs <simon@2ndquadrant.com> wrote: > > On 28 March 2018 at 12:00, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > > > >> v27 attached, though review changes are in > >> the add-on 0005 patch. > > > > This all looks good now, thanks for making all of those changes. > > > > I propose [v27 patch1+patch3+patch5] as the initial commit candidate > > for MERGE, with other patches following later before end CF. > > > > I propose to commit this tomorrow, 30 March, about 26 hours from now. > > That will allow some time for buildfarm fixing/reversion before the > > Easter weekend, then other patches to follow starting 2 April. That > > then gives reasonable time to follow up on other issues that we will > > no doubt discover fairly soon after commit, such as additional runs by > > SQLsmith and more eyeballs. > > No problems found, but moving proposed commit to 2 April pm I did a scan through this, as I hadn't been able to keep with the thread previously. Sorry if some of the things mentioned here have been discussed previously. I am just reading through the patch in its own order, so please excuse if there's things I remark on that only later fully make sense. later update: TL;DR: I don't think the parser / executor implementation of MERGE is architecturally sound. I think creating hidden joins during parse-analysis to implement MERGE is a seriously bad idea and it needs to be replaced by a different executor structure. @@ -3828,8 +3846,14 @@ struct AfterTriggersTableData bool before_trig_done; /* did we already queue BS triggers? */ bool after_trig_done; /* did we already queue AS triggers? */ AfterTriggerEventList after_trig_events; /* if so, saved list pointer */ - Tuplestorestate *old_tuplestore; /* "old" transition table, if any */ - Tuplestorestate *new_tuplestore; /* "new" transition table, if any */ + /* "old" transition table for UPDATE, if any */ + Tuplestorestate *old_upd_tuplestore; + /* "new" transition table for UPDATE, if any */ + Tuplestorestate *new_upd_tuplestore; + /* "old" transition table for DELETE, if any */ + Tuplestorestate *old_del_tuplestore; + /* "new" transition table INSERT, if any */ + Tuplestorestate *new_ins_tuplestore; }; A comment somewhere why we have all of these (presumably because they can now happen in the context of a single statement) would be good. @@ -5744,12 +5796,28 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo, newtup == NULL)); if (oldtup != NULL && - ((event == TRIGGER_EVENT_DELETE && delete_old_table) || - (event == TRIGGER_EVENT_UPDATE && update_old_table))) + (event == TRIGGER_EVENT_DELETE && delete_old_table)) { Tuplestorestate *old_tuplestore; - old_tuplestore = transition_capture->tcs_private->old_tuplestore; + old_tuplestore = transition_capture->tcs_private->old_del_tuplestore; + + if (map != NULL) + { + HeapTuple converted = do_convert_tuple(oldtup, map); + + tuplestore_puttuple(old_tuplestore, converted); + pfree(converted); + } + else + tuplestore_puttuple(old_tuplestore, oldtup); + } Very similar code is now repeated four times. Could you abstract this into a separate function? --- a/src/backend/executor/README +++ b/src/backend/executor/README @@ -37,6 +37,16 @@ the plan tree returns the computed tuples to be updated, plus a "junk" one. For DELETE, the plan tree need only deliver a CTID column, and the ModifyTable node visits each of those rows and marks the row deleted. +MERGE runs one generic plan that returns candidate target rows. Each row +consists of a super-row that contains all the columns needed by any of the +individual actions, plus a CTID and a TABLEOID junk columns. The CTID column is "plus a CTID and a TABLEOID junk columns" sounds a bit awkward? +required to know if a matching target row was found or not and the TABLEOID +column is needed to find the underlying target partition, in case when the +target table is a partition table. If the CTID column is set we attempt to +activate WHEN MATCHED actions, or if it is NULL then we will attempt to "if it is NULL then" sounds wrong. +activate WHEN NOT MATCHED actions. Once we know which action is activated we +form the final result row and apply only those changes. + XXX a great deal more documentation needs to be written here... Are we using tableoids in a similar way in other places? +/* + * Given OID of the partition leaf, return the index of the leaf in the + * partition hierarchy. + */ +int +ExecFindPartitionByOid(PartitionTupleRouting *proute, Oid partoid) +{ + int i; + + for (i = 0; i < proute->num_partitions; i++) + { + if (proute->partition_oids[i] == partoid) + break; + } + + Assert(i < proute->num_partitions); + return i; +} Shouldn't we at least warn in a comment that this is O(N)? And document that it does weird stuff if the OID isn't found? Perhaps just introduce a PARTOID syscache? diff --git a/src/backend/executor/nodeMerge.c b/src/backend/executor/nodeMerge.c new file mode 100644 index 00000000000..0e0d0795d4d --- /dev/null +++ b/src/backend/executor/nodeMerge.c @@ -0,0 +1,575 @@ +/*------------------------------------------------------------------------- + * + * nodeMerge.c + * routines to handle Merge nodes relating to the MERGE command Isn't this file misnamed and it should be execMerge.c? The rest of the node*.c files are for nodes that are invoked via execProcnode / ExecProcNode(). This isn't an actual executor node. Might also be worthwhile to move the merge related init stuff here? What's the reasoning behind making this be an anomaluous type of executor node? +/* + * Perform MERGE. + */ +void +ExecMerge(ModifyTableState *mtstate, EState *estate, TupleTableSlot *slot, + JunkFilter *junkfilter, ResultRelInfo *resultRelInfo) +{ + * ExecMergeMatched takes care of following the update chain and + * re-finding the qualifying WHEN MATCHED action, as long as the updated + * target tuple still satisfies the join quals i.e. it still remains a + * WHEN MATCHED case. If the tuple gets deleted or the join quals fail, it + * returns and we try ExecMergeNotMatched. Given that ExecMergeMatched + * always make progress by following the update chain and we never switch + * from ExecMergeNotMatched to ExecMergeMatched, there is no risk of a + * livelock. + */ + if (matched) + matched = ExecMergeMatched(mtstate, estate, slot, junkfilter, tupleid); + + /* + * Either we were dealing with a NOT MATCHED tuple or ExecMergeNotMatched() + * returned "false", indicating the previously MATCHED tuple is no longer a + * matching tuple. + */ + if (!matched) + ExecMergeNotMatched(mtstate, estate, slot); So what happens if there's a concurrent insertion of a potentially matching tuple? FWIW, I'd re-order this file so this routine is above ExecMergeMatched(), ExecMergeNotMatched(), easier to understand. +static bool +ExecMergeMatched(ModifyTableState *mtstate, EState *estate, + TupleTableSlot *slot, JunkFilter *junkfilter, + ItemPointer tupleid) +{ + ExprContext *econtext = mtstate->ps.ps_ExprContext; + bool isNull; + List *mergeMatchedActionStates = NIL; + HeapUpdateFailureData hufd; + bool tuple_updated, + tuple_deleted; + Buffer buffer; + HeapTupleData tuple; + EPQState *epqstate = &mtstate->mt_epqstate; + ResultRelInfo *saved_resultRelInfo; + ResultRelInfo *resultRelInfo = estate->es_result_relation_info; + ListCell *l; + TupleTableSlot *saved_slot = slot; + + if (mtstate->mt_partition_tuple_routing) + { + Datum datum; + Oid tableoid = InvalidOid; + int leaf_part_index; + PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing; + + /* + * In case of partitioned table, we fetch the tableoid while performing + * MATCHED MERGE action. + */ + datum = ExecGetJunkAttribute(slot, junkfilter->jf_otherJunkAttNo, + &isNull); + Assert(!isNull); + tableoid = DatumGetObjectId(datum); + + /* + * If we're dealing with a MATCHED tuple, then tableoid must have been + * set correctly. In case of partitioned table, we must now fetch the + * correct result relation corresponding to the child table emitting + * the matching target row. For normal table, there is just one result + * relation and it must be the one emitting the matching row. + */ + leaf_part_index = ExecFindPartitionByOid(proute, tableoid); + + resultRelInfo = proute->partitions[leaf_part_index]; + if (resultRelInfo == NULL) + { + resultRelInfo = ExecInitPartitionInfo(mtstate, + mtstate->resultRelInfo, + proute, estate, leaf_part_index); + Assert(resultRelInfo != NULL); + } + } + + /* + * Save the current information and work with the correct result relation. + */ + saved_resultRelInfo = resultRelInfo; + estate->es_result_relation_info = resultRelInfo; + + /* + * And get the correct action lists. + */ + mergeMatchedActionStates = + resultRelInfo->ri_mergeState->matchedActionStates; + + /* + * If there are not WHEN MATCHED actions, we are done. + */ + if (mergeMatchedActionStates == NIL) + return true; Maybe I'm confused, but why is mergeMatchedActionStates attached to per-partition info? How can this differ in number between partitions, requiring us to re-check it below fetching the partition info above? + /* + * Check for any concurrent update/delete operation which may have + * prevented our update/delete. We also check for situations where we + * might be trying to update/delete the same tuple twice. + */ + if ((action->commandType == CMD_UPDATE && !tuple_updated) || + (action->commandType == CMD_DELETE && !tuple_deleted)) + + { + switch (hufd.result) + { + case HeapTupleMayBeUpdated: + break; + case HeapTupleInvisible: + + /* + * This state should never be reached since the underlying + * JOIN runs with a MVCC snapshot and should only return + * rows visible to us. + */ Given EPQ, that reasoning isn't correct. I think this should still be unreachable, just not for the reason described here. + case HeapTupleUpdated: + + /* + * The target tuple was concurrently updated/deleted by + * some other transaction. + * + * If the current tuple is that last tuple in the update + * chain, then we know that the tuple was concurrently + * deleted. Just return and let the caller try NOT MATCHED + * actions. + * + * If the current tuple was concurrently updated, then we + * must run the EvalPlanQual() with the new version of the + * tuple. If EvalPlanQual() does not return a tuple then + * we switch to the NOT MATCHED list of actions. + * If it does return a tuple and the join qual is + * still satisfied, then we just need to recheck the + * MATCHED actions, starting from the top, and execute the + * first qualifying action. + */ + if (!ItemPointerEquals(tupleid, &hufd.ctid)) + { + TupleTableSlot *epqslot; + + /* + * Since we generate a JOIN query with a target table + * RTE different than the result relation RTE, we must + * pass in the RTI of the relation used in the join + * query and not the one from result relation. + */ + Assert(resultRelInfo->ri_mergeTargetRTI > 0); + epqslot = EvalPlanQual(estate, + epqstate, + resultRelInfo->ri_RelationDesc, + GetEPQRangeTableIndex(resultRelInfo), + LockTupleExclusive, + &hufd.ctid, + hufd.xmax); + + if (!TupIsNull(epqslot)) + { + (void) ExecGetJunkAttribute(epqslot, + resultRelInfo->ri_junkFilter->jf_junkAttNo, + &isNull); + + /* + * A non-NULL ctid means that we are still dealing + * with MATCHED case. But we must retry from the + * start with the updated tuple to ensure that the + * first qualifying WHEN MATCHED action is + * executed. + * + * We don't use the new slot returned by + * EvalPlanQual because we anyways re-install the + * new target tuple in econtext->ecxt_scantuple + * before re-evaluating WHEN AND conditions and + * re-projecting the update targetlists. The + * source side tuple does not change and hence we + * can safely continue to use the old slot. + */ + if (!isNull) + { + /* + * Must update *tupleid to the TID of the + * newer tuple found in the update chain. + */ + *tupleid = hufd.ctid; + ReleaseBuffer(buffer); + goto lmerge_matched; It seems fairly bad architecturally to me that we now have EvalPlanQual() loops in both this routine *and* ExecUpdate()/ExecDelete(). + +/* + * Execute the first qualifying NOT MATCHED action. + */ +static void +ExecMergeNotMatched(ModifyTableState *mtstate, EState *estate, + TupleTableSlot *slot) +{ + PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing; + ExprContext *econtext = mtstate->ps.ps_ExprContext; + List *mergeNotMatchedActionStates = NIL; + ResultRelInfo *resultRelInfo; + ListCell *l; + TupleTableSlot *myslot; + + /* + * We are dealing with NOT MATCHED tuple. Since for MERGE, partition tree *the partition tree + * is not expanded for the result relation, we continue to work with the + * currently active result relation, which should be of the root of the + * partition tree. + */ + resultRelInfo = mtstate->resultRelInfo; "should be"? "is", I hope? Given that it's referencing mtstate->resultRelInfo which is only set in one place... +/* flags for mt_merge_subcommands */ +#define MERGE_INSERT 0x01 +#define MERGE_UPDATE 0x02 +#define MERGE_DELETE 0x04 Hm, it seems a bit weird to define these file-local, given there's another file implementing a good chunk of merge. * ExecWithCheckOptions() will skip any WCOs which are not of the kind @@ -617,10 +627,19 @@ ExecInsert(ModifyTableState *mtstate, * passed to foreign table triggers; it is NULL when the foreign * table has no relevant triggers. * + * MERGE passes actionState of the action it's currently executing; + * regular DELETE passes NULL. This is used by ExecDelete to know if it's + * being called from MERGE or regular DELETE operation. + * + * If the DELETE fails because the tuple is concurrently updated/deleted + * by this or some other transaction, hufdp is filled with the reason as + * well as other important information. Currently only MERGE needs this + * information. + * * Returns RETURNING result if any, otherwise NULL. * ---------------------------------------------------------------- */ -static TupleTableSlot * +TupleTableSlot * ExecDelete(ModifyTableState *mtstate, ItemPointer tupleid, HeapTuple oldtuple, @@ -629,6 +648,8 @@ ExecDelete(ModifyTableState *mtstate, EState *estate, bool *tupleDeleted, bool processReturning, + HeapUpdateFailureData *hufdp, + MergeActionState *actionState, bool canSetTag) { ResultRelInfo *resultRelInfo; @@ -641,6 +662,14 @@ ExecDelete(ModifyTableState *mtstate, if (tupleDeleted) *tupleDeleted = false; + /* + * Initialize hufdp. Since the caller is only interested in the failure + * status, initialize with the state that is used to indicate successful + * operation. + */ + if (hufdp) + hufdp->result = HeapTupleMayBeUpdated; + This signals for me that the addition of the result field to HUFD wasn't architecturally the right thing. HUFD is supposed to be supposed to be returned by heap_update(), reusing and setting it from other places seems like a layering violation to me. diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 69dd327f0c9..cd540a0df5b 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -851,6 +851,60 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) fix_scan_list(root, splan->exclRelTlist, rtoffset); } + /* + * The MERGE produces the target rows by performing a right s/the MERGE/the MERGE statement/? + * join between the target relation and the source relation + * (which could be a plain relation or a subquery). The INSERT + * and UPDATE actions of the MERGE requires access to the same. +opt_and_condition: Renaming this to be a bit more merge specific seems like a good idea. +/*------------------------------------------------------------------------- + * + * parse_merge.c + * handle merge-statement in parser +/* + * Special handling for MERGE statement is required because we assemble + * the query manually. This is similar to setTargetTable() followed + * by transformFromClause() but with a few less steps. + * + * Process the FROM clause and add items to the query's range table, + * joinlist, and namespace. + * + * A special targetlist comprising of the columns from the right-subtree of + * the join is populated and returned. Note that when the JoinExpr is + * setup by transformMergeStmt, the left subtree has the target result + * relation and the right subtree has the source relation. + * + * Returns the rangetable index of the target relation. + */ +static int +transformMergeJoinClause(ParseState *pstate, Node *merge, + List **mergeSourceTargetList) +{ + RangeTblEntry *rte, + *rt_rte; + List *namespace; + int rtindex, + rt_rtindex; + Node *n; + int mergeTarget_relation = list_length(pstate->p_rtable) + 1; + Var *var; + TargetEntry *te; + + n = transformFromClauseItem(pstate, merge, + &rte, + &rtindex, + &rt_rte, + &rt_rtindex, + &namespace); + + pstate->p_joinlist = list_make1(n); + + /* + * We created an internal join between the target and the source relation + * to carry out the MERGE actions. Normally such an unaliased join hides + * the joining relations, unless the column references are qualified. + * Also, any unqualified column references are resolved to the Join RTE, if + * there is a matching entry in the targetlist. But the way MERGE + * execution is later setup, we expect all column references to resolve to + * either the source or the target relation. Hence we must not add the + * Join RTE to the namespace. + * + * The last entry must be for the top-level Join RTE. We don't want to + * resolve any references to the Join RTE. So discard that. + * + * We also do not want to resolve any references from the leftside of the + * Join since that corresponds to the target relation. References to the + * columns of the target relation must be resolved from the result + * relation and not the one that is used in the join. So the + * mergeTarget_relation is marked invisible to both qualified as well as + * unqualified references. + */ *Gulp*. I'm extremely doubtful this is a sane approach. At the very least the amount of hackery required to make existing code cope with the approach / duplicate code seems indicative of that. + Assert(list_length(namespace) > 1); + namespace = list_truncate(namespace, list_length(namespace) - 1); + pstate->p_namespace = list_concat(pstate->p_namespace, namespace); + + setNamespaceVisibilityForRTE(pstate->p_namespace, + rt_fetch(mergeTarget_relation, pstate->p_rtable), false, false); + + /* + * Expand the right relation and add its columns to the + * mergeSourceTargetList. Note that the right relation can either be a + * plain relation or a subquery or anything that can have a + * RangeTableEntry. + */ + *mergeSourceTargetList = expandSourceTL(pstate, rt_rte, rt_rtindex); + + /* + * Add a whole-row-Var entry to support references to "source.*". + */ + var = makeWholeRowVar(rt_rte, rt_rtindex, 0, false); + te = makeTargetEntry((Expr *) var, list_length(*mergeSourceTargetList) + 1, + NULL, true); Why can't this properly dealt with by transformWholeRowRef() etc? +/* + * transformMergeStmt - + * transforms a MERGE statement + */ +Query * +transformMergeStmt(ParseState *pstate, MergeStmt *stmt) +{ + Query *qry = makeNode(Query); + ListCell *l; + AclMode targetPerms = ACL_NO_RIGHTS; + bool is_terminal[2]; + JoinExpr *joinexpr; + RangeTblEntry *resultRelRTE, *mergeRelRTE; + + /* There can't be any outer WITH to worry about */ + Assert(pstate->p_ctenamespace == NIL); + + qry->commandType = CMD_MERGE; + + /* + * Check WHEN clauses for permissions and sanity + */ + is_terminal[0] = false; + is_terminal[1] = false; + foreach(l, stmt->mergeActionList) + { + MergeAction *action = (MergeAction *) lfirst(l); + uint when_type = (action->matched ? 0 : 1); + + /* + * Collect action types so we can check Target permissions + */ + switch (action->commandType) + { + case CMD_INSERT: + { + InsertStmt *istmt = (InsertStmt *) action->stmt; + SelectStmt *selectStmt = (SelectStmt *) istmt->selectStmt; + + /* + * The grammar allows attaching ORDER BY, LIMIT, FOR + * UPDATE, or WITH to a VALUES clause and also multiple + * VALUES clauses. If we have any of those, ERROR. + */ + if (selectStmt && (selectStmt->valuesLists == NIL || + selectStmt->sortClause != NIL || + selectStmt->limitOffset != NULL || + selectStmt->limitCount != NULL || + selectStmt->lockingClause != NIL || + selectStmt->withClause != NULL)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("SELECT not allowed in MERGE INSERT statement"))); + if (selectStmt && list_length(selectStmt->valuesLists) > 1) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("Multiple VALUES clauses not allowed in MERGE INSERT statement"))); Shouldn't this include an error position? + /* + * Construct a query of the form + * SELECT relation.ctid --junk attribute + * ,relation.tableoid --junk attribute + * ,source_relation.<somecols> + * ,relation.<somecols> + * FROM relation RIGHT JOIN source_relation + * ON join_condition; -- no WHERE clause - all conditions are applied in + * executor + * + * stmt->relation is the target relation, given as a RangeVar + * stmt->source_relation is a RangeVar or subquery + * + * We specify the join as a RIGHT JOIN as a simple way of forcing the + * first (larg) RTE to refer to the target table. + * + * The MERGE query's join can be tuned in some cases, see below for these + * special case tweaks. + * + * We set QSRC_PARSER to show query constructed in parse analysis + * + * Note that we have only one Query for a MERGE statement and the planner + * is called only once. That query is executed once to produce our stream + * of candidate change rows, so the query must contain all of the columns + * required by each of the targetlist or conditions for each action. + * + * As top-level statements INSERT, UPDATE and DELETE have a Query, whereas + * with MERGE the individual actions do not require separate planning, + * only different handling in the executor. See nodeModifyTable handling + * of commandType CMD_MERGE. + * + * A sub-query can include the Target, but otherwise the sub-query cannot + * reference the outermost Target table at all. + */ + qry->querySource = QSRC_PARSER; Why is this, and not building a proper executor node for merge that knows how to get the tuples, the right approach? We did a rough equivalent for matview updates, and I think it turned out to be a pretty bad plan. + /* + * XXX MERGE is unsupported in various cases + */ + if (!(pstate->p_target_relation->rd_rel->relkind == RELKIND_RELATION || + pstate->p_target_relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("MERGE is not supported for this relation type"))); Shouldn't this report what relation type the error talking about? It's not going to necessarily be obvious to the user. Also, errposition etc would be good. + if (pstate->p_target_relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE && + pstate->p_target_relation->rd_rel->relhassubclass) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("MERGE is not supported for relations with inheritance"))); Hm. + if (pstate->p_target_relation->rd_rel->relhasrules) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("MERGE is not supported for relations with rules"))); I guess we can add that later, but it's a bit sad that this won't work against views with INSTEAD OF triggers. + foreach(l, stmt->mergeActionList) + { + MergeAction *action = (MergeAction *) lfirst(l); + + /* + * Set namespace for the specific action. This must be done before + * analyzing the WHEN quals and the action targetlisst. + */ + setNamespaceForMergeAction(pstate, action); + + /* + * Transform the when condition. + * + * Note that these quals are NOT added to the join quals; instead they + * are evaluated separately during execution to decide which of the + * WHEN MATCHED or WHEN NOT MATCHED actions to execute. + */ + action->qual = transformWhereClause(pstate, action->condition, + EXPR_KIND_MERGE_WHEN_AND, "WHEN"); + + /* + * Transform target lists for each INSERT and UPDATE action stmt + */ + switch (action->commandType) + { + case CMD_INSERT: + { + InsertStmt *istmt = (InsertStmt *) action->stmt; + SelectStmt *selectStmt = (SelectStmt *) istmt->selectStmt; + List *exprList = NIL; + ListCell *lc; + RangeTblEntry *rte; + ListCell *icols; + ListCell *attnos; + List *icolumns; + List *attrnos; + + pstate->p_is_insert = true; + + icolumns = checkInsertTargets(pstate, istmt->cols, &attrnos); + Assert(list_length(icolumns) == list_length(attrnos)); + + /* + * Handle INSERT much like in transformInsertStmt + */ + if (selectStmt == NULL) + { + /* + * We have INSERT ... DEFAULT VALUES. We can handle + * this case by emitting an empty targetlist --- all + * columns will be defaulted when the planner expands + * the targetlist. + */ + exprList = NIL; + } + else + { + /* + * Process INSERT ... VALUES with a single VALUES + * sublist. We treat this case separately for + * efficiency. The sublist is just computed directly + * as the Query's targetlist, with no VALUES RTE. So + * it works just like a SELECT without any FROM. + */ + List *valuesLists = selectStmt->valuesLists; + + Assert(list_length(valuesLists) == 1); + Assert(selectStmt->intoClause == NULL); + + /* + * Do basic expression transformation (same as a ROW() + * expr, but allow SetToDefault at top level) + */ + exprList = transformExpressionList(pstate, + (List *) linitial(valuesLists), + EXPR_KIND_VALUES_SINGLE, + true); + + /* Prepare row for assignment to target table */ + exprList = transformInsertRow(pstate, exprList, + istmt->cols, + icolumns, attrnos, + false); + } Can't we handle this with a littlebit less code duplication? diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 4c0256b18a4..608f50b0616 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -53,23 +53,34 @@ typedef enum LockTupleMode * When heap_update, heap_delete, or heap_lock_tuple fail because the target * tuple is already outdated, they fill in this struct to provide information * to the caller about what happened. + * + * result is the result of HeapTupleSatisfiesUpdate, leading to the failure. + * It's set to HeapTupleMayBeUpdated when there is no failure. + * * ctid is the target's ctid link: it is the same as the target's TID if the * target was deleted, or the location of the replacement tuple if the target * was updated. + * * xmax is the outdating transaction's XID. If the caller wants to visit the * replacement tuple, it must check that this matches before believing the * replacement is really a match. + * * cmax is the outdating command's CID, but only when the failure code is * HeapTupleSelfUpdated (i.e., something in the current transaction outdated * the tuple); otherwise cmax is zero. (We make this restriction because * HeapTupleHeaderGetCmax doesn't work for tuples outdated in other * transactions.) + * + * lockmode is only relevant for callers of heap_update() and is the mode which + * the caller should use in case it needs to lock the updated tuple. */ typedef struct HeapUpdateFailureData { + HTSU_Result result; ItemPointerData ctid; TransactionId xmax; CommandId cmax; + LockTupleMode lockmode; } HeapUpdateFailureData; These new fields seem not really relateto HUFD, but rather just fields the merge code should maintain? Greetings, Andres Freund -
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-04-03T02:40:12Z
On Mon, Apr 2, 2018 at 7:18 PM, Andres Freund <andres@anarazel.de> wrote: > I did a scan through this, as I hadn't been able to keep with the thread > previously. Sorry if some of the things mentioned here have been > discussed previously. I am just reading through the patch in its own > order, so please excuse if there's things I remark on that only later > fully make sense. > > > later update: TL;DR: I don't think the parser / executor implementation > of MERGE is architecturally sound. I think creating hidden joins during > parse-analysis to implement MERGE is a seriously bad idea and it needs > to be replaced by a different executor structure. +1. I continue to have significant misgivings about this. It has many consequences that we know about, and likely quite a few more that we don't. > So what happens if there's a concurrent insertion of a potentially > matching tuple? It's not a special case. In all likelihood, you get a dup violation. This is a behavior that I argued for from an early stage. > It seems fairly bad architecturally to me that we now have > EvalPlanQual() loops in both this routine *and* > ExecUpdate()/ExecDelete(). I think that that makes sense, because ExecMerge() doesn't use any of the EPQ stuff from ExecUpdate() and ExecDelete(). It did at one point, in fact, and it looked quite a lot worse IMV. > *Gulp*. I'm extremely doubtful this is a sane approach. At the very > least the amount of hackery required to make existing code cope with > the approach / duplicate code seems indicative of that. I made a lot of the fact that there are two RTEs for the target, since that has fairly obvious consequences during every stage of query processing. However, I think you're right that the problem is broader than that. > + if (pstate->p_target_relation->rd_rel->relhasrules) > + ereport(ERROR, > + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("MERGE is not supported for relations with rules"))); > > I guess we can add that later, but it's a bit sad that this won't work > against views with INSTEAD OF triggers. It also makes it hard to test deparsing support, which actually made it in in the end. That must be untested. -- Peter Geoghegan -
Re: [HACKERS] MERGE SQL Statement for PG11
Robert Haas <robertmhaas@gmail.com> — 2018-04-03T03:01:50Z
On Mon, Apr 2, 2018 at 10:40 PM, Peter Geoghegan <pg@bowt.ie> wrote: > On Mon, Apr 2, 2018 at 7:18 PM, Andres Freund <andres@anarazel.de> wrote: >> I did a scan through this, as I hadn't been able to keep with the thread >> previously. Sorry if some of the things mentioned here have been >> discussed previously. I am just reading through the patch in its own >> order, so please excuse if there's things I remark on that only later >> fully make sense. >> >> >> later update: TL;DR: I don't think the parser / executor implementation >> of MERGE is architecturally sound. I think creating hidden joins during >> parse-analysis to implement MERGE is a seriously bad idea and it needs >> to be replaced by a different executor structure. > > +1. I continue to have significant misgivings about this. It has many > consequences that we know about, and likely quite a few more that we > don't. +1. I didn't understand from Peter's earlier comments that we were doing that, and I agree that it isn't a good design choice. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-04-03T03:11:53Z
On Tue, Apr 3, 2018 at 8:31 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Apr 2, 2018 at 10:40 PM, Peter Geoghegan <pg@bowt.ie> wrote: > > On Mon, Apr 2, 2018 at 7:18 PM, Andres Freund <andres@anarazel.de> > wrote: > >> I did a scan through this, as I hadn't been able to keep with the thread > >> previously. Sorry if some of the things mentioned here have been > >> discussed previously. I am just reading through the patch in its own > >> order, so please excuse if there's things I remark on that only later > >> fully make sense. > >> > >> > >> later update: TL;DR: I don't think the parser / executor implementation > >> of MERGE is architecturally sound. I think creating hidden joins during > >> parse-analysis to implement MERGE is a seriously bad idea and it needs > >> to be replaced by a different executor structure. > > > > +1. I continue to have significant misgivings about this. It has many > > consequences that we know about, and likely quite a few more that we > > don't. > > +1. I didn't understand from Peter's earlier comments that we were > doing that, and I agree that it isn't a good design choice. > > Honestly I don't think Peter ever raised concerns about the join, though I could be missing early discussions when I wasn't paying attention. It's there from day 1. Peter raised concerns about the two RTE stuff which was necessitated when we added support for partitioned table. We discussed that at some length, with your inputs and agreed that it's not necessarily a bad thing and probably the only way to deal with partitioned tables. Personally, I don't see why an internal join is bad. That's what MERGE is doing anyways, so it closely matches with the overall procedure. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Peter Geoghegan <pg@bowt.ie> — 2018-04-03T03:18:26Z
On Mon, Apr 2, 2018 at 8:11 PM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: > Honestly I don't think Peter ever raised concerns about the join, though I > could be missing early discussions when I wasn't paying attention. It's > there from day 1. Peter raised concerns about the two RTE stuff which was > necessitated when we added support for partitioned table. We discussed that > at some length, with your inputs and agreed that it's not necessarily a bad > thing and probably the only way to deal with partitioned tables. > > Personally, I don't see why an internal join is bad. That's what MERGE is > doing anyways, so it closely matches with the overall procedure. The issue is not that there is a join as such. It's how it's represented in the parser, and how that affects other things. There is a lot of special case logic to make it work. -- Peter Geoghegan
-
Re: [HACKERS] MERGE SQL Statement for PG11
Andres Freund <andres@anarazel.de> — 2018-04-03T03:22:40Z
Hi, On 2018-04-02 19:40:12 -0700, Peter Geoghegan wrote: > > So what happens if there's a concurrent insertion of a potentially > > matching tuple? > > It's not a special case. In all likelihood, you get a dup violation. > This is a behavior that I argued for from an early stage. Right. I think that should be mentioned in the comment... Greetings, Andres Freund
-
Re: [HACKERS] MERGE SQL Statement for PG11
Jesper Pedersen <jesper.pedersen@redhat.com> — 2018-04-04T12:46:15Z
Hi Simon, On 03/30/2018 07:10 AM, Simon Riggs wrote: > No problems found, but moving proposed commit to 2 April pm > There is a warning for this, as attached. Best regards, Jesper
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-04-05T06:01:48Z
On Tue, Apr 3, 2018 at 7:48 AM, Andres Freund <andres@anarazel.de> wrote: > > > @@ -3828,8 +3846,14 @@ struct AfterTriggersTableData > bool before_trig_done; /* did we already queue BS triggers? */ > bool after_trig_done; /* did we already queue AS triggers? */ > AfterTriggerEventList after_trig_events; /* if so, saved list > pointer */ > - Tuplestorestate *old_tuplestore; /* "old" transition table, if any > */ > - Tuplestorestate *new_tuplestore; /* "new" transition table, if any > */ > + /* "old" transition table for UPDATE, if any */ > + Tuplestorestate *old_upd_tuplestore; > + /* "new" transition table for UPDATE, if any */ > + Tuplestorestate *new_upd_tuplestore; > + /* "old" transition table for DELETE, if any */ > + Tuplestorestate *old_del_tuplestore; > + /* "new" transition table INSERT, if any */ > + Tuplestorestate *new_ins_tuplestore; > }; > > A comment somewhere why we have all of these (presumably because they > can now happen in the context of a single statement) would be good. > > Done. > > @@ -5744,12 +5796,28 @@ AfterTriggerSaveEvent(EState *estate, > ResultRelInfo *relinfo, > newtup == NULL)); > > if (oldtup != NULL && > - ((event == TRIGGER_EVENT_DELETE && delete_old_table) || > - (event == TRIGGER_EVENT_UPDATE && update_old_table))) > + (event == TRIGGER_EVENT_DELETE && delete_old_table)) > { > Tuplestorestate *old_tuplestore; > > - old_tuplestore = transition_capture->tcs_privat > e->old_tuplestore; > + old_tuplestore = transition_capture->tcs_privat > e->old_del_tuplestore; > + > + if (map != NULL) > + { > + HeapTuple converted = do_convert_tuple(oldtup, map); > + > + tuplestore_puttuple(old_tuplestore, converted); > + pfree(converted); > + } > + else > + tuplestore_puttuple(old_tuplestore, oldtup); > + } > > Very similar code is now repeated four times. Could you abstract this > into a separate function? > > Ok. I gave it a try, please check. It's definitely a lot lesser code. > > > --- a/src/backend/executor/README > +++ b/src/backend/executor/README > @@ -37,6 +37,16 @@ the plan tree returns the computed tuples to be > updated, plus a "junk" > one. For DELETE, the plan tree need only deliver a CTID column, and the > ModifyTable node visits each of those rows and marks the row deleted. > > +MERGE runs one generic plan that returns candidate target rows. Each row > +consists of a super-row that contains all the columns needed by any of the > +individual actions, plus a CTID and a TABLEOID junk columns. The CTID > column is > > "plus a CTID and a TABLEOID junk columns" sounds a bit awkward? > Changed. > > +required to know if a matching target row was found or not and the > TABLEOID > +column is needed to find the underlying target partition, in case when the > +target table is a partition table. If the CTID column is set we attempt to > +activate WHEN MATCHED actions, or if it is NULL then we will attempt to > > "if it is NULL then" sounds wrong. > Made some adjustments. > > +activate WHEN NOT MATCHED actions. Once we know which action is activated > we > +form the final result row and apply only those changes. > + > XXX a great deal more documentation needs to be written here... > > Are we using tableoids in a similar way in other places? > > AFAIK, no. > > > +/* > + * Given OID of the partition leaf, return the index of the leaf in the > + * partition hierarchy. > + */ > +int > +ExecFindPartitionByOid(PartitionTupleRouting *proute, Oid partoid) > +{ > + int i; > + > + for (i = 0; i < proute->num_partitions; i++) > + { > + if (proute->partition_oids[i] == partoid) > + break; > + } > + > + Assert(i < proute->num_partitions); > + return i; > +} > > Shouldn't we at least warn in a comment that this is O(N)? And document > that it does weird stuff if the OID isn't found? > Yeah, added a comment. Also added a ereport(ERROR) if we do not find the partition. There was already an Assert, but may be ERROR is better. > > Perhaps just introduce a PARTOID syscache? > > Probably as a separate patch. Anything more than a handful partitions is anyways known to be too slow and I doubt this code will add anything material impact to that. > > > diff --git a/src/backend/executor/nodeMerge.c > b/src/backend/executor/nodeMerge.c > new file mode 100644 > index 00000000000..0e0d0795d4d > --- /dev/null > +++ b/src/backend/executor/nodeMerge.c > @@ -0,0 +1,575 @@ > +/*--------------------------------------------------------- > ---------------- > + * > + * nodeMerge.c > + * routines to handle Merge nodes relating to the MERGE command > > Isn't this file misnamed and it should be execMerge.c? The rest of the > node*.c files are for nodes that are invoked via execProcnode / > ExecProcNode(). This isn't an actual executor node. > Makes sense. Done. (Now that the patch is committed, I don't know if there would be a rethink about changing file names. May be not, just raising that concern) > > Might also be worthwhile to move the merge related init stuff here? > Done. > > > What's the reasoning behind making this be an anomaluous type of > executor node? > > Didn't quite get that. I think naming of the file was bad (fixed now), but I think it's a good idea to move the new code in a new file, from maintainability as well as coverage perspective. If you've something very different in mind, can you explain in more details? > > > FWIW, I'd re-order this file so this routine is above > ExecMergeMatched(), ExecMergeNotMatched(), easier to understand. > Done. > > > + /* > + * If there are not WHEN MATCHED actions, we are done. > + */ > + if (mergeMatchedActionStates == NIL) > + return true; > > Maybe I'm confused, but why is mergeMatchedActionStates attached to > per-partition info? How can this differ in number between partitions, > requiring us to re-check it below fetching the partition info above? > > Because each partition may have a columns in different order, dropped attributes etc. So we need to give treatment to the quals/targetlists. See ON CONFLICT DO UPDATE for similar code. > > + /* > + * Check for any concurrent update/delete operation which may have > + * prevented our update/delete. We also check for situations where > we > + * might be trying to update/delete the same tuple twice. > + */ > + if ((action->commandType == CMD_UPDATE && !tuple_updated) || > + (action->commandType == CMD_DELETE && !tuple_deleted)) > + > + { > + switch (hufd.result) > + { > + case HeapTupleMayBeUpdated: > + break; > + case HeapTupleInvisible: > + > + /* > + * This state should never be reached since the > underlying > + * JOIN runs with a MVCC snapshot and should only > return > + * rows visible to us. > + */ > > Given EPQ, that reasoning isn't correct. I think this should still be > unreachable, just not for the reason described here. > > Agree. Updated the comment, but please check if it's satisfactory or you would like to say something more/different. > > > It seems fairly bad architecturally to me that we now have > EvalPlanQual() loops in both this routine *and* > ExecUpdate()/ExecDelete(). > > This was done after review by Peter and I think I like the new way too. Also keeps the regular UPDATE/DELETE code paths least changed and let Merge handle concurrency issues specific to it. > > + > +/* > + * Execute the first qualifying NOT MATCHED action. > + */ > +static void > +ExecMergeNotMatched(ModifyTableState *mtstate, EState *estate, > + TupleTableSlot *slot) > +{ > + PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing; > + ExprContext *econtext = mtstate->ps.ps_ExprContext; > + List *mergeNotMatchedActionStates = NIL; > + ResultRelInfo *resultRelInfo; > + ListCell *l; > + TupleTableSlot *myslot; > + > + /* > + * We are dealing with NOT MATCHED tuple. Since for MERGE, partition > tree > > *the partition tree > Fixed. > > > + * is not expanded for the result relation, we continue to work with > the > + * currently active result relation, which should be of the root of the > + * partition tree. > + */ > + resultRelInfo = mtstate->resultRelInfo; > > "should be"? "is", I hope? Given that it's referencing > mtstate->resultRelInfo which is only set in one place... > > Yeah, fixed. > > > +/* flags for mt_merge_subcommands */ > +#define MERGE_INSERT 0x01 > +#define MERGE_UPDATE 0x02 > +#define MERGE_DELETE 0x04 > > Hm, it seems a bit weird to define these file-local, given there's > another file implementing a good chunk of merge. > Ok. Moved them execMerge.h, which made sense after I moved the initialisation related code to execMerge.c > > + /* > + * Initialize hufdp. Since the caller is only interested in the failure > + * status, initialize with the state that is used to indicate > successful > + * operation. > + */ > + if (hufdp) > + hufdp->result = HeapTupleMayBeUpdated; > + > > This signals for me that the addition of the result field to HUFD wasn't > architecturally the right thing. HUFD is supposed to be supposed to be > returned by heap_update(), reusing and setting it from other places > seems like a layering violation to me. > > I am not sure I agree. Sure we can keep adding more parameters to ExecUpdate/ExecDelete and such routines, but I thought passing back all information via a single struct makes more sense. > > > diff --git a/src/backend/optimizer/plan/setrefs.c > b/src/backend/optimizer/plan/setrefs.c > index 69dd327f0c9..cd540a0df5b 100644 > --- a/src/backend/optimizer/plan/setrefs.c > +++ b/src/backend/optimizer/plan/setrefs.c > @@ -851,6 +851,60 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int > rtoffset) > fix_scan_list(root, splan->exclRelTlist, rtoffset); > } > > + /* > + * The MERGE produces the target rows by performing a right > > s/the MERGE/the MERGE statement/? > > Fixed. > > + * join between the target relation and the source relation > + * (which could be a plain relation or a subquery). The > INSERT > + * and UPDATE actions of the MERGE requires access to the > > same. > > Fixed. > > > +opt_and_condition: > > Renaming this to be a bit more merge specific seems like a good idea. > Renamed to opt_merge_when_and_condition > > + > + /* > + * Add a whole-row-Var entry to support references to "source.*". > + */ > + var = makeWholeRowVar(rt_rte, rt_rtindex, 0, false); > + te = makeTargetEntry((Expr *) var, list_length(*mergeSourceTargetList) > + 1, > + NULL, true); > > Why can't this properly dealt with by transformWholeRowRef() etc? > > I just followed ON CONFLICT style. May be there is a better way, but not clear how. > > > + if (selectStmt && (selectStmt->valuesLists == NIL || > + selectStmt->sortClause != NIL || > + selectStmt->limitOffset != NULL || > + selectStmt->limitCount != NULL || > + selectStmt->lockingClause != NIL || > + selectStmt->withClause != NULL)) > + ereport(ERROR, > + (errcode(ERRCODE_SYNTAX_ERROR), > + errmsg("SELECT not allowed in MERGE > INSERT statement"))); > > + if (selectStmt && list_length(selectStmt->valuesLists) > > 1) > + ereport(ERROR, > + (errcode(ERRCODE_SYNTAX_ERROR), > + errmsg("Multiple VALUES clauses not > allowed in MERGE INSERT statement"))); > > Shouldn't this include an error position? > > I will work on this as a separate follow-up patch. I tried adding location to MergeAction, but that alone is probably not sufficient. So it was turning out a slightly bigger patch than I anticipated. > > Why is this, and not building a proper executor node for merge that > knows how to get the tuples, the right approach? We did a rough > equivalent for matview updates, and I think it turned out to be a pretty > bad plan. > > I am still not sure why that would be any better. Can you explain in detail what exactly you've in mind and how's that significantly better than what we have today? > > > + /* > + * XXX MERGE is unsupported in various cases > + */ > + if (!(pstate->p_target_relation->rd_rel->relkind == RELKIND_RELATION > || > + pstate->p_target_relation->rd_rel->relkind == > RELKIND_PARTITIONED_TABLE)) > + ereport(ERROR, > + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("MERGE is not supported for this relation type"))); > > Shouldn't this report what relation type the error talking about? It's > not going to necessarily be obvious to the user. Also, errposition etc > would be good. > > Hmm, ok. There is getRelationTypeDescription() but otherwise I am surprised that I couldn't find a ready way to get string representation of relation kind. Am I missing something? Of course, we can write for the purpose, but wanted to ensure we are not duplicating something already available. > > > + /* > + * Do basic expression transformation (same as a > ROW() > + * expr, but allow SetToDefault at top level) > + */ > + exprList = transformExpressionList(pstate, > + (List *) > linitial(valuesLists), > + > EXPR_KIND_VALUES_SINGLE, > + true); > + > + /* Prepare row for assignment to target table */ > + exprList = transformInsertRow(pstate, exprList, > + istmt->cols, > + icolumns, attrnos, > + false); > + } > > Can't we handle this with a littlebit less code duplication? > Hmm, yeah, that will be good. But given Tom's suggestions on the other thread, I would like to postpone any refactoring here. > > typedef struct HeapUpdateFailureData > { > + HTSU_Result result; > ItemPointerData ctid; > TransactionId xmax; > CommandId cmax; > + LockTupleMode lockmode; > } HeapUpdateFailureData; > > > These new fields seem not really relateto HUFD, but rather just fields > the merge code should maintain? > As I said, we can possibly track those separately. But they all arise as a result of update/delete/lock failure. So I would prefer to keep them along with other fields such as ctid and xmax/cmax. But if you or others insist, I can move them and pass in/out of various function calls where we need to maintain those. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-04-05T07:55:50Z
On 5 April 2018 at 07:01, Pavan Deolasee <pavan.deolasee@gmail.com> wrote: >> +/* >> + * Given OID of the partition leaf, return the index of the leaf in the >> + * partition hierarchy. >> + */ >> +int >> +ExecFindPartitionByOid(PartitionTupleRouting *proute, Oid partoid) >> +{ >> + int i; >> + >> + for (i = 0; i < proute->num_partitions; i++) >> + { >> + if (proute->partition_oids[i] == partoid) >> + break; >> + } >> + >> + Assert(i < proute->num_partitions); >> + return i; >> +} >> >> Shouldn't we at least warn in a comment that this is O(N)? And document >> that it does weird stuff if the OID isn't found? > > > Yeah, added a comment. Also added a ereport(ERROR) if we do not find the > partition. There was already an Assert, but may be ERROR is better. > >> >> >> Perhaps just introduce a PARTOID syscache? >> > > Probably as a separate patch. Anything more than a handful partitions is > anyways known to be too slow and I doubt this code will add anything > material impact to that. There's a few others trying to change that now, so I think we should consider working on this now. PARTOID syscache sounds like a good approach. >> diff --git a/src/backend/executor/nodeMerge.c >> b/src/backend/executor/nodeMerge.c >> new file mode 100644 >> index 00000000000..0e0d0795d4d >> --- /dev/null >> +++ b/src/backend/executor/nodeMerge.c >> @@ -0,0 +1,575 @@ >> >> +/*------------------------------------------------------------------------- >> + * >> + * nodeMerge.c >> + * routines to handle Merge nodes relating to the MERGE command >> >> Isn't this file misnamed and it should be execMerge.c? The rest of the >> node*.c files are for nodes that are invoked via execProcnode / >> ExecProcNode(). This isn't an actual executor node. > > > Makes sense. Done. (Now that the patch is committed, I don't know if there > would be a rethink about changing file names. May be not, just raising that > concern) My review notes suggest a file called execMerge.c. I didn't spot the filename change. I think it's important to do that because there is no executor node called Merge. That is especially confusing because there *is* an executor node called MergeAppend and we want some cognitive distance between those things. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -
Re: [HACKERS] MERGE SQL Statement for PG11
Jesper Pedersen <jesper.pedersen@redhat.com> — 2018-04-05T11:38:07Z
Hi Simon and Paven, On 04/04/2018 08:46 AM, Jesper Pedersen wrote: > On 03/30/2018 07:10 AM, Simon Riggs wrote: >> No problems found, but moving proposed commit to 2 April pm >> > > There is a warning for this, as attached. > Updated version due to latest refactoring. Best regards, Jesper
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-04-05T11:48:50Z
On 5 April 2018 at 12:38, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > Hi Simon and Paven, > > On 04/04/2018 08:46 AM, Jesper Pedersen wrote: >> >> On 03/30/2018 07:10 AM, Simon Riggs wrote: >>> >>> No problems found, but moving proposed commit to 2 April pm >>> >> >> There is a warning for this, as attached. >> > > Updated version due to latest refactoring. Thanks for your input. Removing that seems to prevent compilation. Did something change in between? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-04-05T11:54:31Z
On Thu, Apr 5, 2018 at 5:08 PM, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > Hi Simon and Paven, > > On 04/04/2018 08:46 AM, Jesper Pedersen wrote: > >> On 03/30/2018 07:10 AM, Simon Riggs wrote: >> >>> No problems found, but moving proposed commit to 2 April pm >>> >>> >> There is a warning for this, as attached. >> >> > Updated version due to latest refactoring. > Hi Jesper, The variable would become unused in non-assert builds. I see that. But simply removing it is not a solution and I don't think the code will compile that way. We should either rewrite that assertion or put it inside a #ifdef ASSERT_CHECKING block or simple remove that assertion because we already check for relkind in parse_merge.c. Will check. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Jesper Pedersen <jesper.pedersen@redhat.com> — 2018-04-05T11:56:47Z
Hi, On 04/05/2018 07:48 AM, Simon Riggs wrote: >> Updated version due to latest refactoring. > > Thanks for your input. Removing that seems to prevent compilation. > > Did something change in between? > Updated for non-assert build. Best regards, Jesper
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-04-05T12:04:47Z
On 5 April 2018 at 12:56, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > Hi, > > On 04/05/2018 07:48 AM, Simon Riggs wrote: >>> >>> Updated version due to latest refactoring. >> >> >> Thanks for your input. Removing that seems to prevent compilation. >> >> Did something change in between? >> > > Updated for non-assert build. Thanks, pushed. Sorry to have you wait til v3 -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Teodor Sigaev <teodor@sigaev.ru> — 2018-04-05T12:18:17Z
> The variable would become unused in non-assert builds. I see that. But simply > removing it is not a solution and I don't think the code will compile that way. > We should either rewrite that assertion or put it inside a #ifdef > ASSERT_CHECKING block or simple remove that assertion because we already check > for relkind in parse_merge.c. Will check. > That is noted by Kyotaro HORIGUCHI https://www.postgresql.org/message-id/20180405.181730.125855581.horiguchi.kyotaro%40lab.ntt.co.jp and his suggestion to use special macro looks better for me: - char relkind; + char relkind PG_USED_FOR_ASSERTS_ONLY; -- Teodor Sigaev E-mail: teodor@sigaev.ru WWW: http://www.sigaev.ru/ -
Re: [HACKERS] MERGE SQL Statement for PG11
Jesper Pedersen <jesper.pedersen@redhat.com> — 2018-04-05T12:19:04Z
Hi, On 04/05/2018 08:04 AM, Simon Riggs wrote: > On 5 April 2018 at 12:56, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: >> Updated for non-assert build. > > Thanks, pushed. Sorry to have you wait til v3 > That patch was a but rushed, and cut off too much. As attached. Best regards, Jesper
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-04-05T12:22:04Z
On 5 April 2018 at 13:19, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > Hi, > > On 04/05/2018 08:04 AM, Simon Riggs wrote: >> >> On 5 April 2018 at 12:56, Jesper Pedersen <jesper.pedersen@redhat.com> >> wrote: >>> >>> Updated for non-assert build. >> >> >> Thanks, pushed. Sorry to have you wait til v3 >> > > That patch was a but rushed, and cut off too much. Yes, noted, already fixed. Thanks -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-04-05T12:24:25Z
On 5 April 2018 at 13:18, Teodor Sigaev <teodor@sigaev.ru> wrote: >> The variable would become unused in non-assert builds. I see that. But >> simply removing it is not a solution and I don't think the code will compile >> that way. We should either rewrite that assertion or put it inside a #ifdef >> ASSERT_CHECKING block or simple remove that assertion because we already >> check for relkind in parse_merge.c. Will check. >> > > That is noted by Kyotaro HORIGUCHI > https://www.postgresql.org/message-id/20180405.181730.125855581.horiguchi.kyotaro%40lab.ntt.co.jp > > and his suggestion to use special macro looks better for me: > - char relkind; > + char relkind PG_USED_FOR_ASSERTS_ONLY; Thanks both, I already fixed that. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-05T15:09:15Z
Quick item: parse_clause.h fails cpluspluscheck because it has a C++ keyword as a function argument name: ./src/include/parser/parse_clause.h:26:14: error: expected ‘,’ or ‘...’ before ‘namespace’ List **namespace); ^~~~~~~~~ -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-04-05T15:58:46Z
On 5 April 2018 at 16:09, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Quick item: parse_clause.h fails cpluspluscheck because it has a C++ > keyword as a function argument name: > > ./src/include/parser/parse_clause.h:26:14: error: expected ‘,’ or ‘...’ before ‘namespace’ > List **namespace); > ^~~~~~~~~ How's this as a fix? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-05T16:07:11Z
Simon Riggs wrote: > On 5 April 2018 at 16:09, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Quick item: parse_clause.h fails cpluspluscheck because it has a C++ > > keyword as a function argument name: > > > > ./src/include/parser/parse_clause.h:26:14: error: expected ‘,’ or ‘...’ before ‘namespace’ > > List **namespace); > > ^~~~~~~~~ > > How's this as a fix? WFM -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-04-05T19:11:23Z
On 5 April 2018 at 17:07, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Simon Riggs wrote: >> On 5 April 2018 at 16:09, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> > Quick item: parse_clause.h fails cpluspluscheck because it has a C++ >> > keyword as a function argument name: >> > >> > ./src/include/parser/parse_clause.h:26:14: error: expected ‘,’ or ‘...’ before ‘namespace’ >> > List **namespace); >> > ^~~~~~~~~ >> >> How's this as a fix? > > WFM Pushed -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Andres Freund <andres@anarazel.de> — 2018-04-05T20:00:03Z
Hi, On 2018-04-05 11:31:48 +0530, Pavan Deolasee wrote: > > +/*--------------------------------------------------------- > > ---------------- > > + * > > + * nodeMerge.c > > + * routines to handle Merge nodes relating to the MERGE command > > > > Isn't this file misnamed and it should be execMerge.c? The rest of the > > node*.c files are for nodes that are invoked via execProcnode / > > ExecProcNode(). This isn't an actual executor node. > > > > Makes sense. Done. (Now that the patch is committed, I don't know if there > would be a rethink about changing file names. May be not, just raising that > concern) It absolutely definitely needed to be renamed. But that's been done, so ... > > What's the reasoning behind making this be an anomaluous type of > > executor node? > Didn't quite get that. I think naming of the file was bad (fixed now), but > I think it's a good idea to move the new code in a new file, from > maintainability as well as coverage perspective. If you've something very > different in mind, can you explain in more details? Well, it was kinda modeled as an executor node, including the file name. That's somewhat fixed now. I still am extremely suspicious of the codeflow here. My impression is that this simply shouldn't be going through nodeModifyTuple, but be it's own nodeMerge node. The trigger handling would need to be abstraced into execTrigger.c or such to avoid duplication. We're now going from nodeModifyTable.c:ExecModifyTable() into execMerge.c:ExecMerge(), back to nodeModifyTable.c:ExecUpdate/Insert(). To avoid ExecInsert() doing things that aren't appropriate for merge, we then pass an actionState, which neuters part of ExecUpdate/Insert(). This is just bad. I think this should be cleaned up before it can be released, which means this feature should be reverted and cleaned up nicely before being re-committed. Otherwise we'll sit on this bad architecture and it'll make future features harder. And if somebody cleans it up Simon will complain that things are needlessly being destabilized (hello xlog.c with it's 1000+ LOC functions). > > + /* > > + * If there are not WHEN MATCHED actions, we are done. > > + */ > > + if (mergeMatchedActionStates == NIL) > > + return true; > > > > Maybe I'm confused, but why is mergeMatchedActionStates attached to > > per-partition info? How can this differ in number between partitions, > > requiring us to re-check it below fetching the partition info above? > > > > > Because each partition may have a columns in different order, dropped > attributes etc. So we need to give treatment to the quals/targetlists. See > ON CONFLICT DO UPDATE for similar code. But the count wouldn't change, no? So we return before building the partition info if there's no MATCHED action? > > It seems fairly bad architecturally to me that we now have > > EvalPlanQual() loops in both this routine *and* > > ExecUpdate()/ExecDelete(). > > > > > This was done after review by Peter and I think I like the new way too. > Also keeps the regular UPDATE/DELETE code paths least changed and let Merge > handle concurrency issues specific to it. It also makes the whole code barely readable. Minimal amount of changes isn't a bad goal, but if the consequence of that is poor layering and repeated code it's bad as well. > > + /* > > + * Initialize hufdp. Since the caller is only interested in the failure > > + * status, initialize with the state that is used to indicate > > successful > > + * operation. > > + */ > > + if (hufdp) > > + hufdp->result = HeapTupleMayBeUpdated; > > + > > > > This signals for me that the addition of the result field to HUFD wasn't > > architecturally the right thing. HUFD is supposed to be supposed to be > > returned by heap_update(), reusing and setting it from other places > > seems like a layering violation to me. > I am not sure I agree. Sure we can keep adding more parameters to > ExecUpdate/ExecDelete and such routines, but I thought passing back all > information via a single struct makes more sense. You can just wrap HUFD in another struct that has the necessary information. > > + > > + /* > > + * Add a whole-row-Var entry to support references to "source.*". > > + */ > > + var = makeWholeRowVar(rt_rte, rt_rtindex, 0, false); > > + te = makeTargetEntry((Expr *) var, list_length(*mergeSourceTargetList) > > + 1, > > + NULL, true); > > > > Why can't this properly dealt with by transformWholeRowRef() etc? > > > > > I just followed ON CONFLICT style. May be there is a better way, but not > clear how. What code are you referring to? > > Why is this, and not building a proper executor node for merge that > > knows how to get the tuples, the right approach? We did a rough > > equivalent for matview updates, and I think it turned out to be a pretty > > bad plan. > > > > > I am still not sure why that would be any better. Can you explain in detail > what exactly you've in mind and how's that significantly better than what > we have today? Because the code flow would be understandable, we'd have proper parser locations, we'd not have to introduce a new query source type, the deparsing would be less painful, we'd not have to optimizer like hijinks in parse analysis etc. In my opinion you're attempting to do planner / optimizer stuff at the parse-analysis level, and that's just not a good idea. Greetings, Andres Freund
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-04-06T04:31:58Z
On Fri, Apr 6, 2018 at 1:30 AM, Andres Freund <andres@anarazel.de> wrote: > > > My impression is that this simply shouldn't be going through > nodeModifyTuple, but be it's own nodeMerge node. The trigger handling > would need to be abstraced into execTrigger.c or such to avoid > duplication. We're now going from nodeModifyTable.c:ExecModifyTable() > into execMerge.c:ExecMerge(), back to > nodeModifyTable.c:ExecUpdate/Insert(). To avoid ExecInsert() doing > things that aren't appropriate for merge, we then pass an actionState, > which neuters part of ExecUpdate/Insert(). This is just bad. > > But wouldn't this lead to lot of code duplication? For example, ExecInsert/ExecUpdate does a bunch of supporting work (firing triggers, inserting into indexes just to name a few) that MERGE's INSERT/UPDATE needs as well. Now we can possibly move these support routines to a new file, say execModify.c and then let both Merge as well as ModifyTable node make use of that. But the fact is that ExecInsert/ExecUpdate knows a lot about ModifyTable already. So to separate ExecInsert/ExecUpdate from ModifyTable will require significant refactoring AFAICS. I am not saying we can't do that, but will have it's own consequences. If we would not have refactored to move ExecMerge and friends to a new file, then it may not have looked so odd (as you describe above). But I think moving the code to a new file was a net improvement. May be we can move ExecInsert/Update etc to a new file as I suggested, but still use the ModifyTable to run Merge. There are many things common between them. ModifyTable executes all DMLs and MERGE is just another DML which can run all three. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-04-06T07:38:43Z
On 2018/04/06 5:00, Andres Freund wrote: > On 2018-04-05 11:31:48 +0530, Pavan Deolasee wrote: >>> + /* >>> + * If there are not WHEN MATCHED actions, we are done. >>> + */ >>> + if (mergeMatchedActionStates == NIL) >>> + return true; >>> >>> Maybe I'm confused, but why is mergeMatchedActionStates attached to >>> per-partition info? How can this differ in number between partitions, >>> requiring us to re-check it below fetching the partition info above? >>> >>> >> Because each partition may have a columns in different order, dropped >> attributes etc. So we need to give treatment to the quals/targetlists. See >> ON CONFLICT DO UPDATE for similar code. > > But the count wouldn't change, no? So we return before building the > partition info if there's no MATCHED action? Yeah, I think we should return at the top if there are no matched actions. With the current code, even if there aren't any matched actions we're doing ExecFindPartitionByOid() and ExecInitPartitionInfo() to only then see in the partition's resultRelInfo that the action states list is empty. I think there should be an Assert where there currently is the check for empty action states list and the check itself should be at the top of the function, as done in the attached. Thanks, Amit
-
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs <simon@2ndquadrant.com> — 2018-04-06T09:35:34Z
On 5 April 2018 at 21:00, Andres Freund <andres@anarazel.de> wrote: > And if somebody cleans it up Simon will > complain that things are needlessly being destabilized (hello xlog.c > with it's 1000+ LOC functions). Taking this comment as a special point... with other points addressed separately. ...If anybody wants to know what I think they can just ask... There are various parts of the code that don't have full test coverage. Changing things there is more dangerous and if its being done for no reason then that is risk for little reward. That doesn't block it, but it does make me think that people could spend their time better - and issue that concerns me also. But that certainly doesn't apply to parts of the code like this where we have full test coverage. It may not even apply to recovery now we have the ability to check in real-time the results of recovery and replication. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [HACKERS] MERGE SQL Statement for PG11
Pavan Deolasee <pavan.deolasee@gmail.com> — 2018-04-06T11:52:19Z
On Fri, Apr 6, 2018 at 1:30 AM, Andres Freund <andres@anarazel.de> wrote: > Hi, > > On 2018-04-05 11:31:48 +0530, Pavan Deolasee wrote: > > > +/*--------------------------------------------------------- > > > ---------------- > > > + * > > > + * nodeMerge.c > > > + * routines to handle Merge nodes relating to the MERGE command > > > > > > Isn't this file misnamed and it should be execMerge.c? The rest of the > > > node*.c files are for nodes that are invoked via execProcnode / > > > ExecProcNode(). This isn't an actual executor node. > > > > > > > Makes sense. Done. (Now that the patch is committed, I don't know if > there > > would be a rethink about changing file names. May be not, just raising > that > > concern) > > It absolutely definitely needed to be renamed. But that's been done, so > ... > > > > > What's the reasoning behind making this be an anomaluous type of > > > executor node? > > > Didn't quite get that. I think naming of the file was bad (fixed now), > but > > I think it's a good idea to move the new code in a new file, from > > maintainability as well as coverage perspective. If you've something very > > different in mind, can you explain in more details? > > Well, it was kinda modeled as an executor node, including the file > name. That's somewhat fixed now. I still am extremely suspicious of > the codeflow here. > > My impression is that this simply shouldn't be going through > nodeModifyTuple, but be it's own nodeMerge node. The trigger handling > would need to be abstraced into execTrigger.c There is nothing called execTrigger.c. You probably mean creating something new or trigger.c. That being said, I don't think the current code is bad. There is already a switch to handle different command types. We add one more for CMD_MERGE and then fire individual BEFORE/AFTER statement triggers based on what actions MERGE may take. The ROW trigger handling doesn't require any change. > or such to avoid > duplication. We're now going from nodeModifyTable.c:ExecModifyTable() > into execMerge.c:ExecMerge(), back to > nodeModifyTable.c:ExecUpdate/Insert(). To avoid ExecInsert() doing > things that aren't appropriate for merge, we then pass an actionState, > which neuters part of ExecUpdate/Insert(). This is just bad. > I have spent most part of today trying to hack around the executor with the goal to do what you're suggesting. Having done so, I must admit I am actually quite puzzled why you think having a new node is going to be any significant improvement. I first tried to split nodeModifyTable.c into two parts. One that deals with just ModifyTable node (the exported Init/Exec/End functions to start with) and the other which abstracts out the actual Insert/Update/Delete operations. The idea was that the second part shouldn't know anything about ModifyTable and it being just one of the users. I started increasingly disliking the result as I continued hacking. The knowledge of ModifyTable node is quite wide-spread, including execPartition.c and FDW. The ExecInsert/Update, partitioning, ON CONFLICT handling all make use of ModifyTable extensively. While MERGE does not need ON CONFLICT, it needs everything else, even FDW at some point in future. Do you agree that this is a bad choice or am I getting it completely wrong or do you really expect MERGE to completely refactor nodeModifyTable.c, including the partitioning related code? I gave up on this approach. I then started working on other possibility where we keep a ModifyTable node inside a MergeTable (that's the name I am using, for now). The paths/plans/executor state gets built that way. So all common members still belong to the ModifyTable (and friends) and we only add MERGE specific information to MergeTable (and friends). This, at least looks somewhat better. We can have: - ExecInitMergeTable(): does some basic initialisation of the embedded ModifyTable node so that it can be passed around - ExecMergeTable(): executes the (only) subplan, fetches tuples, fires BR triggers, does work for handling transition tables (so mostly duplication of what ExecModifyTable does already) and then executes the MERGE actions. It will mostly use the ModifyTable node created during initialisation when calling ExecUpdate/Insert etc - ExecEndMergeTable(): ends the executor This is probably far better than the first approach. But is it really a huge improvement over the committed code? Or even an improvement at all? If that's not what you've in mind, can you please explain in more detail how to you see the final design and how exactly it's better than what we have today? Once there is clarity, I can work on it in a fairly quick manner. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services