public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/2] Expose GUC flags in SQL function; retire ./check_guc 5+ messages / 4 participants [nested] [flat]
* [PATCH 2/2] Expose GUC flags in SQL function; retire ./check_guc @ 2021-12-08 18:06 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2021-12-08 18:06 UTC (permalink / raw) --- src/backend/catalog/system_views.sql | 19 ++++- src/backend/utils/misc/check_guc | 29 ------- src/backend/utils/misc/guc.c | 37 +++++++- src/include/catalog/pg_proc.dat | 6 +- src/test/regress/expected/guc.out | 122 +++++++++++++++++++++++++++ src/test/regress/expected/rules.out | 2 +- src/test/regress/sql/guc.sql | 72 ++++++++++++++++ 7 files changed, 252 insertions(+), 35 deletions(-) delete mode 100755 src/backend/utils/misc/check_guc diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 3cb69b1f87b..cdae0616bdd 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -582,7 +582,24 @@ FROM JOIN pg_authid rol ON l.classoid = rol.tableoid AND l.objoid = rol.oid; CREATE VIEW pg_settings AS - SELECT * FROM pg_show_all_settings() AS A; + SELECT a.name, + a.setting, + a.unit, + a.category, + a.short_desc, + a.extra_desc, + a.context, + a.vartype, + a.source, + a.min_val, + a.max_val, + a.enumvals, + a.boot_val, + a.reset_val, + a.sourcefile, + a.sourceline, + a.pending_restart + FROM pg_show_all_settings() AS A; CREATE RULE pg_settings_u AS ON UPDATE TO pg_settings diff --git a/src/backend/utils/misc/check_guc b/src/backend/utils/misc/check_guc deleted file mode 100755 index 323ca13191b..00000000000 --- a/src/backend/utils/misc/check_guc +++ /dev/null @@ -1,29 +0,0 @@ -#! /bin/sh -set -e - -## this script makes some assumptions about the formatting of files it parses. -## in postgresql.conf.sample: -## 1) the valid config settings may be preceded by a '#', but NOT '# ' -## (we use this to skip comments) - -### What options are listed in postgresql.conf.sample, but don't appear -### in guc.c? - -# grab everything that looks like a setting -SETTINGS=`sed '/^#[[:alnum:]]/!d; s/^#//; s/ =.*//; /^include/d' postgresql.conf.sample` - -for i in $SETTINGS ; do - ## it sure would be nice to replace this with an sql "not in" statement - grep -i "\"$i\"" guc.c >/dev/null || - echo "$i seems to be missing from guc.c"; -done - -### What options are listed in guc.c, but don't appear -### in postgresql.conf.sample? - -# grab everything that looks like a setting and convert it to lower case -SETTINGS=`gawk -F '[",]' 'BEGIN{RS="\n\t\\\\{\n"} /",[[:space:]]*PGC_.*.*gettext_noop/ && !/NOT_IN_SAMPLE/{print tolower($2)}' guc.c` -for i in $SETTINGS ; do - grep "#$i " postgresql.conf.sample >/dev/null || - echo "$i seems to be missing from postgresql.conf.sample"; -done diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 4c94f09c645..2cb84d68610 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -9634,6 +9634,36 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok) return _ShowOption(record, true); } +/* + * Return an text[] array of the given flags (or a useful subset?) XXX + */ +static char * +get_flags_array_text(int flags) +{ + StringInfoData ret; + + initStringInfo(&ret); + appendStringInfoChar(&ret, '{'); + + if (flags & GUC_NO_SHOW_ALL) + appendStringInfo(&ret, "NO_SHOW_ALL,"); + if (flags & GUC_NO_RESET_ALL) + appendStringInfo(&ret, "NO_RESET_ALL,"); + if (flags & GUC_NOT_IN_SAMPLE) + appendStringInfo(&ret, "NOT_IN_SAMPLE,"); + if (flags & GUC_EXPLAIN) + appendStringInfo(&ret, "EXPLAIN,"); + if (flags & GUC_RUNTIME_COMPUTED) + appendStringInfo(&ret, "RUNTIME_COMPUTED,"); + + /* Remove trailing comma, if any */ + if (ret.len > 1) + ret.data[--ret.len] = '\0'; + + appendStringInfoChar(&ret, '}'); + return ret.data; +} + /* * Return GUC variable value by variable number; optionally return canonical * form of name. Return value is palloc'd. @@ -9861,6 +9891,9 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow) } values[16] = (conf->status & GUC_PENDING_RESTART) ? "t" : "f"; + + /* flags */ + values[17] = get_flags_array_text(conf->flags); } /* @@ -9916,7 +9949,7 @@ show_config_by_name_missing_ok(PG_FUNCTION_ARGS) * show_all_settings - equiv to SHOW ALL command but implemented as * a Table Function. */ -#define NUM_PG_SETTINGS_ATTS 17 +#define NUM_PG_SETTINGS_ATTS 18 Datum show_all_settings(PG_FUNCTION_ARGS) @@ -9978,6 +10011,8 @@ show_all_settings(PG_FUNCTION_ARGS) INT4OID, -1, 0); TupleDescInitEntry(tupdesc, (AttrNumber) 17, "pending_restart", BOOLOID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 18, "flags", + TEXTARRAYOID, -1, 0); /* * Generate attribute metadata needed later to produce tuples from raw diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 0859dc81cac..1d7f6b4d376 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -6092,9 +6092,9 @@ { oid => '2084', descr => 'SHOW ALL as a function', proname => 'pg_show_all_settings', prorows => '1000', proretset => 't', provolatile => 's', prorettype => 'record', proargtypes => '', - proallargtypes => '{text,text,text,text,text,text,text,text,text,text,text,_text,text,text,text,int4,bool}', - proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', - proargnames => '{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline,pending_restart}', + proallargtypes => '{text,text,text,text,text,text,text,text,text,text,text,_text,text,text,text,int4,bool,_text}', + proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline,pending_restart,flags}', prosrc => 'show_all_settings' }, { oid => '3329', descr => 'show config file settings', proname => 'pg_show_all_file_settings', prorows => '1000', proretset => 't', diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 59da91ff04d..5f15b5680be 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -813,3 +813,125 @@ set default_with_oids to f; -- Should not allow to set it to true. set default_with_oids to t; ERROR: tables declared WITH OIDS are not supported +-- +-- Test GUC categories and flags. +-- +CREATE TABLE pg_settings_flags AS SELECT name, category, + 'NO_SHOW_ALL' =ANY(flags) AS no_show_all, + 'NO_RESET_ALL' =ANY(flags) AS no_reset_all, + 'NOT_IN_SAMPLE' =ANY(flags) AS not_in_sample, + 'EXPLAIN' =ANY(flags) AS guc_explain, + 'COMPUTED' =ANY(flags) AS guc_computed + FROM pg_show_all_settings(); +-- test that GUCS are in postgresql.conf +SELECT lower(name) FROM pg_settings_flags WHERE NOT not_in_sample EXCEPT +SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc +FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'), '\n') AS ln) conf +WHERE ln ~ '^#?[[:alpha:]]' +ORDER BY 1; + lower +----------------------------- + config_file + plpgsql.check_asserts + plpgsql.extra_errors + plpgsql.extra_warnings + plpgsql.print_strict_params + plpgsql.variable_conflict +(6 rows) + +-- test that lines in postgresql.conf that look like GUCs are GUCs +SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc +FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'), '\n') AS ln) conf +WHERE ln ~ '^#?[[:alpha:]]' +EXCEPT SELECT lower(name) FROM pg_settings_flags WHERE NOT not_in_sample +ORDER BY 1; + guc +------------------- + include + include_dir + include_if_exists +(3 rows) + +-- test that section:DEVELOPER GUCs are flagged GUC_NOT_IN_SAMPLE: +SELECT * FROM pg_settings_flags +WHERE category='Developer Options' AND NOT not_in_sample +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +------+----------+-------------+--------------+---------------+-------------+-------------- +(0 rows) + +-- Maybe the converse: +SELECT * FROM pg_settings_flags +WHERE category NOT IN ('Developer Options', 'Preset Options') AND not_in_sample +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +------------------------+-------------------------------------------------+-------------+--------------+---------------+-------------+-------------- + application_name | Reporting and Logging / What to Log | f | f | t | f | f + transaction_deferrable | Client Connection Defaults / Statement Behavior | f | t | t | f | f + transaction_isolation | Client Connection Defaults / Statement Behavior | f | t | t | f | f + transaction_read_only | Client Connection Defaults / Statement Behavior | f | t | t | f | f +(4 rows) + +-- Most Query Tuning GUCs are flagged as EXPLAIN: +SELECT * FROM pg_settings_flags +WHERE category ~ '^Query Tuning' AND NOT guc_explain +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +---------------------------+--------------------------------------+-------------+--------------+---------------+-------------+-------------- + default_statistics_target | Query Tuning / Other Planner Options | f | f | f | f | f +(1 row) + +-- Maybe the converse: +SELECT * FROM pg_settings_flags +WHERE guc_explain AND NOT category ~ '^Query Tuning|^Resource Usage' +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +---------------------+-------------------------------------------------+-------------+--------------+---------------+-------------+-------------- + force_parallel_mode | Developer Options | f | f | t | t | f + search_path | Client Connection Defaults / Statement Behavior | f | f | f | t | f +(2 rows) + +-- GUCs flagged RUNTIME are Preset +SELECT * FROM pg_settings_flags +WHERE guc_computed AND NOT category='Preset Options' +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +------+----------+-------------+--------------+---------------+-------------+-------------- +(0 rows) + +-- PRESET GUCs are flagged NOT_IN_SAMPLE +SELECT * FROM pg_settings_flags +WHERE category='Preset Options' AND NOT not_in_sample +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +------+----------+-------------+--------------+---------------+-------------+-------------- +(0 rows) + +-- NO_SHOW_ALL implies NO_RESET_ALL: +SELECT * FROM pg_settings_flags +WHERE no_show_all AND NOT no_reset_all +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +------+----------+-------------+--------------+---------------+-------------+-------------- +(0 rows) + +-- Usually the converse: +SELECT * FROM pg_settings_flags +WHERE NOT no_show_all AND no_reset_all +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +------------------------+-------------------------------------------------+-------------+--------------+---------------+-------------+-------------- + transaction_deferrable | Client Connection Defaults / Statement Behavior | f | t | t | f | f + transaction_isolation | Client Connection Defaults / Statement Behavior | f | t | t | f | f + transaction_read_only | Client Connection Defaults / Statement Behavior | f | t | t | f | f +(3 rows) + +-- NO_SHOW_ALL implies NOT_IN_SAMPLE: +SELECT * FROM pg_settings_flags +WHERE no_show_all AND NOT not_in_sample +ORDER BY 1; + name | category | no_show_all | no_reset_all | not_in_sample | guc_explain | guc_computed +------+----------+-------------+--------------+---------------+-------------+-------------- +(0 rows) + +DROP TABLE pg_settings_flags; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index d652f7b5fb4..833c1fab6c7 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1725,7 +1725,7 @@ pg_settings| SELECT a.name, a.sourcefile, a.sourceline, a.pending_restart - FROM pg_show_all_settings() a(name, setting, unit, category, short_desc, extra_desc, context, vartype, source, min_val, max_val, enumvals, boot_val, reset_val, sourcefile, sourceline, pending_restart); + FROM pg_show_all_settings() a(name, setting, unit, category, short_desc, extra_desc, context, vartype, source, min_val, max_val, enumvals, boot_val, reset_val, sourcefile, sourceline, pending_restart, flags); pg_shadow| SELECT pg_authid.rolname AS usename, pg_authid.oid AS usesysid, pg_authid.rolcreatedb AS usecreatedb, diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index c39c11388d5..c3898ae690f 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -311,3 +311,75 @@ reset check_function_bodies; set default_with_oids to f; -- Should not allow to set it to true. set default_with_oids to t; + +-- +-- Test GUC categories and flags. +-- +CREATE TABLE pg_settings_flags AS SELECT name, category, + 'NO_SHOW_ALL' =ANY(flags) AS no_show_all, + 'NO_RESET_ALL' =ANY(flags) AS no_reset_all, + 'NOT_IN_SAMPLE' =ANY(flags) AS not_in_sample, + 'EXPLAIN' =ANY(flags) AS guc_explain, + 'COMPUTED' =ANY(flags) AS guc_computed + FROM pg_show_all_settings(); + +-- test that GUCS are in postgresql.conf +SELECT lower(name) FROM pg_settings_flags WHERE NOT not_in_sample EXCEPT +SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc +FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'), '\n') AS ln) conf +WHERE ln ~ '^#?[[:alpha:]]' +ORDER BY 1; + +-- test that lines in postgresql.conf that look like GUCs are GUCs +SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc +FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'), '\n') AS ln) conf +WHERE ln ~ '^#?[[:alpha:]]' +EXCEPT SELECT lower(name) FROM pg_settings_flags WHERE NOT not_in_sample +ORDER BY 1; + +-- test that section:DEVELOPER GUCs are flagged GUC_NOT_IN_SAMPLE: +SELECT * FROM pg_settings_flags +WHERE category='Developer Options' AND NOT not_in_sample +ORDER BY 1; + +-- Maybe the converse: +SELECT * FROM pg_settings_flags +WHERE category NOT IN ('Developer Options', 'Preset Options') AND not_in_sample +ORDER BY 1; + +-- Most Query Tuning GUCs are flagged as EXPLAIN: +SELECT * FROM pg_settings_flags +WHERE category ~ '^Query Tuning' AND NOT guc_explain +ORDER BY 1; + +-- Maybe the converse: +SELECT * FROM pg_settings_flags +WHERE guc_explain AND NOT category ~ '^Query Tuning|^Resource Usage' +ORDER BY 1; + +-- GUCs flagged RUNTIME are Preset +SELECT * FROM pg_settings_flags +WHERE guc_computed AND NOT category='Preset Options' +ORDER BY 1; + +-- PRESET GUCs are flagged NOT_IN_SAMPLE +SELECT * FROM pg_settings_flags +WHERE category='Preset Options' AND NOT not_in_sample +ORDER BY 1; + +-- NO_SHOW_ALL implies NO_RESET_ALL: +SELECT * FROM pg_settings_flags +WHERE no_show_all AND NOT no_reset_all +ORDER BY 1; + +-- Usually the converse: +SELECT * FROM pg_settings_flags +WHERE NOT no_show_all AND no_reset_all +ORDER BY 1; + +-- NO_SHOW_ALL implies NOT_IN_SAMPLE: +SELECT * FROM pg_settings_flags +WHERE no_show_all AND NOT not_in_sample +ORDER BY 1; + +DROP TABLE pg_settings_flags; -- 2.17.1 --EsfvRFssnM00t552-- ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) @ 2024-02-22 16:20 Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2024-02-22 16:20 UTC (permalink / raw) To: Dean Rasheed <[email protected]>; +Cc: zwj <[email protected]>; pgsql-hackers <[email protected]> Dean Rasheed <[email protected]> writes: > Attached is a patch that prevents UNION ALL subquery pullup in MERGE only. I think that this is a band-aid that's just masking an error in the rowmarking logic: it's not doing the right thing for appendrels made from UNION ALL subqueries. I've not wrapped my head around exactly where it's going off the rails, but I feel like this ought to get fixed somewhere else. Please hold off pushing your patch for now. (The test case looks valuable though, thanks for working on that.) regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) @ 2024-02-23 00:12 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2024-02-23 00:12 UTC (permalink / raw) To: Dean Rasheed <[email protected]>; +Cc: zwj <[email protected]>; pgsql-hackers <[email protected]> I wrote: > I think that this is a band-aid that's just masking an error in the > rowmarking logic: it's not doing the right thing for appendrels > made from UNION ALL subqueries. I've not wrapped my head around > exactly where it's going off the rails, but I feel like this ought > to get fixed somewhere else. Please hold off pushing your patch > for now. So after studying this for awhile, I see that the planner is emitting a PlanRowMark that presumes that the UNION ALL subquery will be scanned as though it's a base relation; but since we've converted it to an appendrel, the executor just ignores that rowmark, and the wrong things happen. I think the really right fix would be to teach the executor to honor such PlanRowMarks, by getting nodeAppend.c and nodeMergeAppend.c to perform EPQ row substitution. I wrote a draft patch for that, attached, and it almost works but not quite. The trouble is that we're jamming the contents of the row identity Var created for the rowmark into the output of the Append or MergeAppend, and it doesn't necessarily match exactly. In the test case you created, the planner produces targetlists like Output: src_1.val, src_1.id, ROW(src_1.id, src_1.val) and as you can see the order of the columns doesn't match. I can see three ways we might attack that: 1. Persuade the planner to build output tlists that always match the row identity Var. This seems undesirable, because the planner might have intentionally elided columns that won't be read by the upper parts of the plan. 2. Change generation of the ROW() expression so that it lists only the values we're going to output, in the order we're going to output them. This would amount to saying that for UNION cases the "identity" of a row need only consider columns used by the plan, which feels a little odd but I can't think of a reason why it wouldn't work. I'm not sure how messy this'd be to implement though, as the set of columns to be emitted isn't fully determined until much later than where we currently expand the row identity Vars into RowExprs. 3. Fix the executor to remap what it gets out of the ROW() into the order of the subquery tlists. This is probably do-able but I'm not certain; it may be that the executor hasn't enough info. We might need to teach the planner to produce a mapping projection and attach it to the Append node, which carries some ABI risk (but in the past we've gotten away with adding new fields to the ends of plan nodes in the back branches). Another objection is that adding cycles to execution rather than planning might be a poor tradeoff --- although if we only do the work when EPQ is invoked, maybe it'd be the best way. It might be that any of these things is too messy to be considered for back-patching, and we ought to apply what you did in the back branches. I'd like a better fix in HEAD though. regards, tom lane Attachments: [text/x-diff] appendrel-row-locks-wip.patch (5.7K, ../../[email protected]/2-appendrel-row-locks-wip.patch) download | inline diff: diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c index c7059e7528..113217e607 100644 --- a/src/backend/executor/nodeAppend.c +++ b/src/backend/executor/nodeAppend.c @@ -288,8 +288,65 @@ static TupleTableSlot * ExecAppend(PlanState *pstate) { AppendState *node = castNode(AppendState, pstate); + EState *estate = node->ps.state; TupleTableSlot *result; + if (estate->es_epq_active != NULL) + { + /* + * We are inside an EvalPlanQual recheck. If there is a relevant + * rowmark for the append relation, return the test tuple if one is + * available. + */ + EPQState *epqstate = estate->es_epq_active; + int scanrelid; + + if (bms_get_singleton_member(castNode(Append, node->ps.plan)->apprelids, + &scanrelid)) + { + if (epqstate->relsubs_done[scanrelid - 1]) + { + /* + * Return empty slot, as either there is no EPQ tuple for this + * rel or we already returned it. + */ + TupleTableSlot *slot = node->ps.ps_ResultTupleSlot; + + return ExecClearTuple(slot); + } + else if (epqstate->relsubs_slot[scanrelid - 1] != NULL) + { + /* + * Return replacement tuple provided by the EPQ caller. + */ + TupleTableSlot *slot = epqstate->relsubs_slot[scanrelid - 1]; + + Assert(epqstate->relsubs_rowmark[scanrelid - 1] == NULL); + + /* Mark to remember that we shouldn't return it again */ + epqstate->relsubs_done[scanrelid - 1] = true; + + return slot; + } + else if (epqstate->relsubs_rowmark[scanrelid - 1] != NULL) + { + /* + * Fetch and return replacement tuple using a non-locking + * rowmark. + */ + TupleTableSlot *slot = node->ps.ps_ResultTupleSlot; + + /* Mark to remember that we shouldn't return more */ + epqstate->relsubs_done[scanrelid - 1] = true; + + if (!EvalPlanQualFetchRowMark(epqstate, scanrelid, slot)) + return NULL; + + return slot; + } + } + } + /* * If this is the first call after Init or ReScan, we need to do the * initialization work. @@ -405,6 +462,7 @@ ExecEndAppend(AppendState *node) void ExecReScanAppend(AppendState *node) { + EState *estate = node->ps.state; int nasyncplans = node->as_nasyncplans; int i; @@ -443,6 +501,23 @@ ExecReScanAppend(AppendState *node) ExecReScan(subnode); } + /* + * Rescan EvalPlanQual tuple(s) if we're inside an EvalPlanQual recheck. + * But don't lose the "blocked" status of blocked target relations. + */ + if (estate->es_epq_active != NULL) + { + EPQState *epqstate = estate->es_epq_active; + int scanrelid; + + if (bms_get_singleton_member(castNode(Append, node->ps.plan)->apprelids, + &scanrelid)) + { + epqstate->relsubs_done[scanrelid - 1] = + epqstate->relsubs_blocked[scanrelid - 1]; + } + } + /* Reset async state */ if (nasyncplans > 0) { diff --git a/src/backend/executor/nodeMergeAppend.c b/src/backend/executor/nodeMergeAppend.c index 0817868452..f2c386b123 100644 --- a/src/backend/executor/nodeMergeAppend.c +++ b/src/backend/executor/nodeMergeAppend.c @@ -200,11 +200,68 @@ static TupleTableSlot * ExecMergeAppend(PlanState *pstate) { MergeAppendState *node = castNode(MergeAppendState, pstate); + EState *estate = node->ps.state; TupleTableSlot *result; SlotNumber i; CHECK_FOR_INTERRUPTS(); + if (estate->es_epq_active != NULL) + { + /* + * We are inside an EvalPlanQual recheck. If there is a relevant + * rowmark for the append relation, return the test tuple if one is + * available. + */ + EPQState *epqstate = estate->es_epq_active; + int scanrelid; + + if (bms_get_singleton_member(castNode(MergeAppend, node->ps.plan)->apprelids, + &scanrelid)) + { + if (epqstate->relsubs_done[scanrelid - 1]) + { + /* + * Return empty slot, as either there is no EPQ tuple for this + * rel or we already returned it. + */ + TupleTableSlot *slot = node->ps.ps_ResultTupleSlot; + + return ExecClearTuple(slot); + } + else if (epqstate->relsubs_slot[scanrelid - 1] != NULL) + { + /* + * Return replacement tuple provided by the EPQ caller. + */ + TupleTableSlot *slot = epqstate->relsubs_slot[scanrelid - 1]; + + Assert(epqstate->relsubs_rowmark[scanrelid - 1] == NULL); + + /* Mark to remember that we shouldn't return it again */ + epqstate->relsubs_done[scanrelid - 1] = true; + + return slot; + } + else if (epqstate->relsubs_rowmark[scanrelid - 1] != NULL) + { + /* + * Fetch and return replacement tuple using a non-locking + * rowmark. + */ + TupleTableSlot *slot = node->ps.ps_ResultTupleSlot; + + /* Mark to remember that we shouldn't return more */ + epqstate->relsubs_done[scanrelid - 1] = true; + + if (!EvalPlanQualFetchRowMark(epqstate, scanrelid, slot)) + return NULL; + + return slot; + } + } + } + if (!node->ms_initialized) { /* Nothing to do if all subplans were pruned */ @@ -339,6 +396,7 @@ ExecEndMergeAppend(MergeAppendState *node) void ExecReScanMergeAppend(MergeAppendState *node) { + EState *estate = node->ps.state; int i; /* @@ -372,6 +430,24 @@ ExecReScanMergeAppend(MergeAppendState *node) if (subnode->chgParam == NULL) ExecReScan(subnode); } + + /* + * Rescan EvalPlanQual tuple(s) if we're inside an EvalPlanQual recheck. + * But don't lose the "blocked" status of blocked target relations. + */ + if (estate->es_epq_active != NULL) + { + EPQState *epqstate = estate->es_epq_active; + int scanrelid; + + if (bms_get_singleton_member(castNode(MergeAppend, node->ps.plan)->apprelids, + &scanrelid)) + { + epqstate->relsubs_done[scanrelid - 1] = + epqstate->relsubs_blocked[scanrelid - 1]; + } + } + binaryheap_reset(node->ms_heap); node->ms_initialized = false; } ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) @ 2024-02-27 12:53 Dean Rasheed <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Dean Rasheed @ 2024-02-27 12:53 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: zwj <[email protected]>; pgsql-hackers <[email protected]> On Fri, 23 Feb 2024 at 00:12, Tom Lane <[email protected]> wrote: > > So after studying this for awhile, I see that the planner is emitting > a PlanRowMark that presumes that the UNION ALL subquery will be > scanned as though it's a base relation; but since we've converted it > to an appendrel, the executor just ignores that rowmark, and the wrong > things happen. I think the really right fix would be to teach the > executor to honor such PlanRowMarks, by getting nodeAppend.c and > nodeMergeAppend.c to perform EPQ row substitution. Yes, I agree that's a much better solution, if it can be made to work, though I have been really struggling to see how. > the planner produces targetlists like > > Output: src_1.val, src_1.id, ROW(src_1.id, src_1.val) > > and as you can see the order of the columns doesn't match. > I can see three ways we might attack that: > > 1. Persuade the planner to build output tlists that always match > the row identity Var. > > 2. Change generation of the ROW() expression so that it lists only > the values we're going to output, in the order we're going to > output them. > > 3. Fix the executor to remap what it gets out of the ROW() into the > order of the subquery tlists. This is probably do-able but I'm > not certain; it may be that the executor hasn't enough info. > We might need to teach the planner to produce a mapping projection > and attach it to the Append node, which carries some ABI risk (but > in the past we've gotten away with adding new fields to the ends > of plan nodes in the back branches). Another objection is that > adding cycles to execution rather than planning might be a poor > tradeoff --- although if we only do the work when EPQ is invoked, > maybe it'd be the best way. > Of those, option 3 feels like the best one, though I'm really not sure. I played around with it and convinced myself that the executor doesn't have the information it needs to make it work, but I think all it needs is the Append node's original targetlist, as it is just before it's rewritten by set_dummy_tlist_references(), which rewrites the attribute numbers sequentially. In the original targetlist, all the Vars have the right attribute numbers, so it can be used to build the required projection (I think). Attached is a very rough patch. It seemed better to build the projection in the executor rather than the planner, since then the extra work can be avoided, if EPQ is not invoked. It seems to work (it passes the isolation tests, and I couldn't break it in ad hoc testing), but it definitely needs tidying up, and it's hard to be sure that it's not overlooking something. Regards, Dean Attachments: [text/x-patch] appendrel-row-locks-wip.v2.patch (20.1K, ../../CAEZATCVa-mgPuOdgd8+YVgOJ4wgJuhT2mJznbj_tmsGAP8hHJw@mail.gmail.com/2-appendrel-row-locks-wip.v2.patch) download | inline diff: diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c new file mode 100644 index c7059e7..ccd994c --- a/src/backend/executor/nodeAppend.c +++ b/src/backend/executor/nodeAppend.c @@ -275,6 +275,8 @@ ExecInitAppend(Append *node, EState *est /* For parallel query, this will be overridden later. */ appendstate->choose_next_subplan = choose_next_subplan_locally; + appendstate->as_epq_tupdesc = NULL; + return appendstate; } @@ -288,8 +290,107 @@ static TupleTableSlot * ExecAppend(PlanState *pstate) { AppendState *node = castNode(AppendState, pstate); + EState *estate = node->ps.state; TupleTableSlot *result; + if (estate->es_epq_active != NULL) + { + /* + * We are inside an EvalPlanQual recheck. If there is a relevant + * rowmark for the append relation, return the test tuple if one is + * available. + */ + EPQState *epqstate = estate->es_epq_active; + int scanrelid; + + if (bms_get_singleton_member(castNode(Append, node->ps.plan)->apprelids, + &scanrelid)) + { + if (epqstate->relsubs_done[scanrelid - 1]) + { + /* + * Return empty slot, as either there is no EPQ tuple for this + * rel or we already returned it. + */ + TupleTableSlot *slot = node->ps.ps_ResultTupleSlot; + + return ExecClearTuple(slot); + } + else if (epqstate->relsubs_slot[scanrelid - 1] != NULL) + { + /* + * Return replacement tuple provided by the EPQ caller. + */ + TupleTableSlot *slot = epqstate->relsubs_slot[scanrelid - 1]; + + Assert(epqstate->relsubs_rowmark[scanrelid - 1] == NULL); + + /* Mark to remember that we shouldn't return it again */ + epqstate->relsubs_done[scanrelid - 1] = true; + + return slot; + } + else if (epqstate->relsubs_rowmark[scanrelid - 1] != NULL) + { + /* + * Fetch and return replacement tuple using a non-locking + * rowmark. + */ + ExecAuxRowMark *earm = epqstate->relsubs_rowmark[scanrelid - 1]; + ExecRowMark *erm = earm->rowmark; + Datum datum; + bool isNull; + TupleTableSlot *slot; + + Assert(erm->markType == ROW_MARK_COPY); + + datum = ExecGetJunkAttribute(epqstate->origslot, + earm->wholeAttNo, + &isNull); + if (isNull) + return NULL; + + if (node->as_epq_tupdesc == NULL) + { + HeapTupleHeader tuple; + Oid tupType; + int32 tupTypmod; + MemoryContext oldcontext; + + tuple = DatumGetHeapTupleHeader(datum); + tupType = HeapTupleHeaderGetTypeId(tuple); + tupTypmod = HeapTupleHeaderGetTypMod(tuple); + + oldcontext = MemoryContextSwitchTo(estate->es_query_cxt); + + node->as_epq_tupdesc = lookup_rowtype_tupdesc_copy(tupType, tupTypmod); + + ExecAssignExprContext(estate, &node->ps); + + node->ps.ps_ProjInfo = + ExecBuildProjectionInfo(castNode(Append, node->ps.plan)->epq_targetlist, + node->ps.ps_ExprContext, + node->ps.ps_ResultTupleSlot, + &node->ps, + NULL); + + MemoryContextSwitchTo(oldcontext); + } + + slot = MakeTupleTableSlot(node->as_epq_tupdesc, + &TTSOpsVirtual); + ExecStoreHeapTupleDatum(datum, slot); + + /* Mark to remember that we shouldn't return more */ + epqstate->relsubs_done[scanrelid - 1] = true; + + node->ps.ps_ExprContext->ecxt_scantuple = slot; + + return ExecProject(node->ps.ps_ProjInfo); + } + } + } + /* * If this is the first call after Init or ReScan, we need to do the * initialization work. @@ -405,6 +506,7 @@ ExecEndAppend(AppendState *node) void ExecReScanAppend(AppendState *node) { + EState *estate = node->ps.state; int nasyncplans = node->as_nasyncplans; int i; @@ -443,6 +545,23 @@ ExecReScanAppend(AppendState *node) ExecReScan(subnode); } + /* + * Rescan EvalPlanQual tuple(s) if we're inside an EvalPlanQual recheck. + * But don't lose the "blocked" status of blocked target relations. + */ + if (estate->es_epq_active != NULL) + { + EPQState *epqstate = estate->es_epq_active; + int scanrelid; + + if (bms_get_singleton_member(castNode(Append, node->ps.plan)->apprelids, + &scanrelid)) + { + epqstate->relsubs_done[scanrelid - 1] = + epqstate->relsubs_blocked[scanrelid - 1]; + } + } + /* Reset async state */ if (nasyncplans > 0) { diff --git a/src/backend/executor/nodeMergeAppend.c b/src/backend/executor/nodeMergeAppend.c new file mode 100644 index 0817868..2c0caee --- a/src/backend/executor/nodeMergeAppend.c +++ b/src/backend/executor/nodeMergeAppend.c @@ -187,6 +187,8 @@ ExecInitMergeAppend(MergeAppend *node, E */ mergestate->ms_initialized = false; + mergestate->ms_epq_tupdesc = NULL; + return mergestate; } @@ -200,11 +202,110 @@ static TupleTableSlot * ExecMergeAppend(PlanState *pstate) { MergeAppendState *node = castNode(MergeAppendState, pstate); + EState *estate = node->ps.state; TupleTableSlot *result; SlotNumber i; CHECK_FOR_INTERRUPTS(); + if (estate->es_epq_active != NULL) + { + /* + * We are inside an EvalPlanQual recheck. If there is a relevant + * rowmark for the append relation, return the test tuple if one is + * available. + */ + EPQState *epqstate = estate->es_epq_active; + int scanrelid; + + if (bms_get_singleton_member(castNode(MergeAppend, node->ps.plan)->apprelids, + &scanrelid)) + { + if (epqstate->relsubs_done[scanrelid - 1]) + { + /* + * Return empty slot, as either there is no EPQ tuple for this + * rel or we already returned it. + */ + TupleTableSlot *slot = node->ps.ps_ResultTupleSlot; + + return ExecClearTuple(slot); + } + else if (epqstate->relsubs_slot[scanrelid - 1] != NULL) + { + /* + * Return replacement tuple provided by the EPQ caller. + */ + TupleTableSlot *slot = epqstate->relsubs_slot[scanrelid - 1]; + + Assert(epqstate->relsubs_rowmark[scanrelid - 1] == NULL); + + /* Mark to remember that we shouldn't return it again */ + epqstate->relsubs_done[scanrelid - 1] = true; + + return slot; + } + else if (epqstate->relsubs_rowmark[scanrelid - 1] != NULL) + { + /* + * Fetch and return replacement tuple using a non-locking + * rowmark. + */ + ExecAuxRowMark *earm = epqstate->relsubs_rowmark[scanrelid - 1]; + ExecRowMark *erm = earm->rowmark; + Datum datum; + bool isNull; + TupleTableSlot *slot; + + Assert(erm->markType == ROW_MARK_COPY); + + datum = ExecGetJunkAttribute(epqstate->origslot, + earm->wholeAttNo, + &isNull); + if (isNull) + return NULL; + + if (node->ms_epq_tupdesc == NULL) + { + HeapTupleHeader tuple; + Oid tupType; + int32 tupTypmod; + MemoryContext oldcontext; + + tuple = DatumGetHeapTupleHeader(datum); + tupType = HeapTupleHeaderGetTypeId(tuple); + tupTypmod = HeapTupleHeaderGetTypMod(tuple); + + oldcontext = MemoryContextSwitchTo(estate->es_query_cxt); + + node->ms_epq_tupdesc = lookup_rowtype_tupdesc_copy(tupType, tupTypmod); + + ExecAssignExprContext(estate, &node->ps); + + node->ps.ps_ProjInfo = + ExecBuildProjectionInfo(castNode(MergeAppend, node->ps.plan)->epq_targetlist, + node->ps.ps_ExprContext, + node->ps.ps_ResultTupleSlot, + &node->ps, + NULL); + + MemoryContextSwitchTo(oldcontext); + } + + slot = MakeTupleTableSlot(node->ms_epq_tupdesc, + &TTSOpsVirtual); + ExecStoreHeapTupleDatum(datum, slot); + + /* Mark to remember that we shouldn't return more */ + epqstate->relsubs_done[scanrelid - 1] = true; + + node->ps.ps_ExprContext->ecxt_scantuple = slot; + + return ExecProject(node->ps.ps_ProjInfo); + } + } + } + if (!node->ms_initialized) { /* Nothing to do if all subplans were pruned */ @@ -339,6 +440,7 @@ ExecEndMergeAppend(MergeAppendState *nod void ExecReScanMergeAppend(MergeAppendState *node) { + EState *estate = node->ps.state; int i; /* @@ -372,6 +474,24 @@ ExecReScanMergeAppend(MergeAppendState * if (subnode->chgParam == NULL) ExecReScan(subnode); } + + /* + * Rescan EvalPlanQual tuple(s) if we're inside an EvalPlanQual recheck. + * But don't lose the "blocked" status of blocked target relations. + */ + if (estate->es_epq_active != NULL) + { + EPQState *epqstate = estate->es_epq_active; + int scanrelid; + + if (bms_get_singleton_member(castNode(MergeAppend, node->ps.plan)->apprelids, + &scanrelid)) + { + epqstate->relsubs_done[scanrelid - 1] = + epqstate->relsubs_blocked[scanrelid - 1]; + } + } + binaryheap_reset(node->ms_heap); node->ms_initialized = false; } diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c new file mode 100644 index 22a1fa2..bb07ca4 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -1761,8 +1761,9 @@ set_append_references(PlannerInfo *root, /* * Otherwise, clean up the Append as needed. It's okay to do this after * recursing to the children, because set_dummy_tlist_references doesn't - * look at those. + * look at those. Save the original targetlist for EPQ checking. */ + aplan->epq_targetlist = aplan->plan.targetlist; set_dummy_tlist_references((Plan *) aplan, rtoffset); aplan->apprelids = offset_relid_set(aplan->apprelids, rtoffset); @@ -1837,8 +1838,9 @@ set_mergeappend_references(PlannerInfo * /* * Otherwise, clean up the MergeAppend as needed. It's okay to do this * after recursing to the children, because set_dummy_tlist_references - * doesn't look at those. + * doesn't look at those. Save the original targetlist for EPQ checking. */ + mplan->epq_targetlist = mplan->plan.targetlist; set_dummy_tlist_references((Plan *) mplan, rtoffset); mplan->apprelids = offset_relid_set(mplan->apprelids, rtoffset); diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h new file mode 100644 index 444a5f0..6c3ca6e --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -1377,6 +1377,7 @@ struct AppendState Bitmapset *as_valid_subplans; Bitmapset *as_valid_asyncplans; /* valid asynchronous plans indexes */ bool (*choose_next_subplan) (AppendState *); + TupleDesc as_epq_tupdesc; /* wholerow tuple descriptor for EPQ check */ }; /* ---------------- @@ -1406,6 +1407,7 @@ typedef struct MergeAppendState bool ms_initialized; /* are subplans started? */ struct PartitionPruneState *ms_prune_state; Bitmapset *ms_valid_subplans; + TupleDesc ms_epq_tupdesc; /* wholerow tuple descriptor for EPQ check */ } MergeAppendState; /* ---------------- diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h new file mode 100644 index b4ef6bc..e178175 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -275,6 +275,8 @@ typedef struct Append /* Info for run-time subplan pruning; NULL if we're not doing that */ struct PartitionPruneInfo *part_prune_info; + + List *epq_targetlist; /* Targetlist for EPQ checking */ } Append; /* ---------------- @@ -310,6 +312,8 @@ typedef struct MergeAppend /* Info for run-time subplan pruning; NULL if we're not doing that */ struct PartitionPruneInfo *part_prune_info; + + List *epq_targetlist; /* Targetlist for EPQ checking */ } MergeAppend; /* ---------------- diff --git a/src/test/isolation/expected/merge-join.out b/src/test/isolation/expected/merge-join.out new file mode 100644 index 57f048c..a9b49a7 --- a/src/test/isolation/expected/merge-join.out +++ b/src/test/isolation/expected/merge-join.out @@ -146,3 +146,144 @@ id|val 3| 30 (3 rows) + +starting permutation: b1 b2 m1 hj exu m2u c1 c2 s1 +step b1: BEGIN ISOLATION LEVEL READ COMMITTED; +step b2: BEGIN ISOLATION LEVEL READ COMMITTED; +step m1: MERGE INTO tgt USING src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); +step hj: SET LOCAL enable_mergejoin = off; SET LOCAL enable_nestloop = off; +step exu: EXPLAIN (verbose, costs off) + MERGE INTO tgt USING (SELECT * FROM src + UNION ALL + SELECT * FROM src2) src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); +QUERY PLAN +------------------------------------------------------------------------- +Merge on public.tgt + -> Hash Left Join + Output: tgt.ctid, src_1.val, src_1.id, (ROW(src_1.id, src_1.val)) + Inner Unique: true + Hash Cond: (src_1.id = tgt.id) + -> Append + -> Seq Scan on public.src src_1 + Output: src_1.val, src_1.id, ROW(src_1.id, src_1.val) + -> Seq Scan on public.src2 + Output: src2.val, src2.id, ROW(src2.id, src2.val) + -> Hash + Output: tgt.ctid, tgt.id + -> Seq Scan on public.tgt + Output: tgt.ctid, tgt.id +(14 rows) + +step m2u: MERGE INTO tgt USING (SELECT * FROM src + UNION ALL + SELECT * FROM src2) src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); <waiting ...> +step c1: COMMIT; +step m2u: <... completed> +step c2: COMMIT; +step s1: SELECT * FROM tgt; +id|val +--+--- + 1| 10 + 2| 20 + 3| 30 +(3 rows) + + +starting permutation: b1 b2 m1 mj exu m2u c1 c2 s1 +step b1: BEGIN ISOLATION LEVEL READ COMMITTED; +step b2: BEGIN ISOLATION LEVEL READ COMMITTED; +step m1: MERGE INTO tgt USING src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); +step mj: SET LOCAL enable_hashjoin = off; SET LOCAL enable_nestloop = off; +step exu: EXPLAIN (verbose, costs off) + MERGE INTO tgt USING (SELECT * FROM src + UNION ALL + SELECT * FROM src2) src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); +QUERY PLAN +------------------------------------------------------------------------- +Merge on public.tgt + -> Merge Left Join + Output: tgt.ctid, src_1.val, src_1.id, (ROW(src_1.id, src_1.val)) + Inner Unique: true + Merge Cond: (src_1.id = tgt.id) + -> Merge Append + Sort Key: src_1.id + -> Index Scan using src_pkey on public.src src_1 + Output: src_1.val, src_1.id, ROW(src_1.id, src_1.val) + -> Index Scan using src2_pkey on public.src2 + Output: src2.val, src2.id, ROW(src2.id, src2.val) + -> Index Scan using tgt_pkey on public.tgt + Output: tgt.ctid, tgt.id +(13 rows) + +step m2u: MERGE INTO tgt USING (SELECT * FROM src + UNION ALL + SELECT * FROM src2) src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); <waiting ...> +step c1: COMMIT; +step m2u: <... completed> +step c2: COMMIT; +step s1: SELECT * FROM tgt; +id|val +--+--- + 1| 10 + 2| 20 + 3| 30 +(3 rows) + + +starting permutation: b1 b2 m1 nl exu m2u c1 c2 s1 +step b1: BEGIN ISOLATION LEVEL READ COMMITTED; +step b2: BEGIN ISOLATION LEVEL READ COMMITTED; +step m1: MERGE INTO tgt USING src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); +step nl: SET LOCAL enable_hashjoin = off; SET LOCAL enable_mergejoin = off; +step exu: EXPLAIN (verbose, costs off) + MERGE INTO tgt USING (SELECT * FROM src + UNION ALL + SELECT * FROM src2) src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); +QUERY PLAN +------------------------------------------------------------------------- +Merge on public.tgt + -> Nested Loop Left Join + Output: tgt.ctid, src_1.val, src_1.id, (ROW(src_1.id, src_1.val)) + Inner Unique: true + -> Append + -> Seq Scan on public.src src_1 + Output: src_1.val, src_1.id, ROW(src_1.id, src_1.val) + -> Seq Scan on public.src2 + Output: src2.val, src2.id, ROW(src2.id, src2.val) + -> Index Scan using tgt_pkey on public.tgt + Output: tgt.ctid, tgt.id + Index Cond: (tgt.id = src_1.id) +(12 rows) + +step m2u: MERGE INTO tgt USING (SELECT * FROM src + UNION ALL + SELECT * FROM src2) src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); <waiting ...> +step c1: COMMIT; +step m2u: <... completed> +step c2: COMMIT; +step s1: SELECT * FROM tgt; +id|val +--+--- + 1| 10 + 2| 20 + 3| 30 +(3 rows) + diff --git a/src/test/isolation/specs/merge-join.spec b/src/test/isolation/specs/merge-join.spec new file mode 100644 index e33a02c..564702b --- a/src/test/isolation/specs/merge-join.spec +++ b/src/test/isolation/specs/merge-join.spec @@ -6,6 +6,7 @@ setup { CREATE TABLE src (id int PRIMARY KEY, val int); + CREATE TABLE src2 (id int PRIMARY KEY, val int); CREATE TABLE tgt (id int PRIMARY KEY, val int); INSERT INTO src SELECT x, x*10 FROM generate_series(1,3) g(x); INSERT INTO tgt SELECT x, x FROM generate_series(1,3) g(x); @@ -13,7 +14,7 @@ setup teardown { - DROP TABLE src, tgt; + DROP TABLE src, src2, tgt; } session s1 @@ -36,6 +37,17 @@ step ex { EXPLAIN (verbose, costs off) step m2 { MERGE INTO tgt USING src ON tgt.id = src.id WHEN MATCHED THEN UPDATE SET val = src.val WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); } +step exu { EXPLAIN (verbose, costs off) + MERGE INTO tgt USING (SELECT * FROM src + UNION ALL + SELECT * FROM src2) src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); } +step m2u { MERGE INTO tgt USING (SELECT * FROM src + UNION ALL + SELECT * FROM src2) src ON tgt.id = src.id + WHEN MATCHED THEN UPDATE SET val = src.val + WHEN NOT MATCHED THEN INSERT VALUES (src.id, src.val); } step s2 { SELECT * FROM tgt; } step c2 { COMMIT; } @@ -43,3 +55,6 @@ permutation b1 m1 s1 c1 b2 m2 s2 c2 permutation b1 b2 m1 hj ex m2 c1 c2 s1 permutation b1 b2 m1 mj ex m2 c1 c2 s1 permutation b1 b2 m1 nl ex m2 c1 c2 s1 +permutation b1 b2 m1 hj exu m2u c1 c2 s1 +permutation b1 b2 m1 mj exu m2u c1 c2 s1 +permutation b1 b2 m1 nl exu m2u c1 c2 s1 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) @ 2024-02-28 09:16 jian he <[email protected]> parent: Dean Rasheed <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: jian he @ 2024-02-28 09:16 UTC (permalink / raw) To: Dean Rasheed <[email protected]>; +Cc: Tom Lane <[email protected]>; zwj <[email protected]>; pgsql-hackers <[email protected]> On Tue, Feb 27, 2024 at 8:53 PM Dean Rasheed <[email protected]> wrote: > > Attached is a very rough patch. It seemed better to build the > projection in the executor rather than the planner, since then the > extra work can be avoided, if EPQ is not invoked. > > It seems to work (it passes the isolation tests, and I couldn't break > it in ad hoc testing), but it definitely needs tidying up, and it's > hard to be sure that it's not overlooking something. > Hi. minor issues. In nodeAppend.c, nodeMergeAppend.c + if (estate->es_epq_active != NULL) + { + /* + * We are inside an EvalPlanQual recheck. If there is a relevant + * rowmark for the append relation, return the test tuple if one is + * available. + */ + oldcontext = MemoryContextSwitchTo(estate->es_query_cxt); + + node->as_epq_tupdesc = lookup_rowtype_tupdesc_copy(tupType, tupTypmod); + + ExecAssignExprContext(estate, &node->ps); + + node->ps.ps_ProjInfo = + ExecBuildProjectionInfo(castNode(Append, node->ps.plan)->epq_targetlist, + node->ps.ps_ExprContext, + node->ps.ps_ResultTupleSlot, + &node->ps, + NULL); + + MemoryContextSwitchTo(oldcontext); EvalPlanQualStart, EvalPlanQualNext will switch the memory context to es_query_cxt. so the memory context switch here is not necessary? do you think it's sane to change `ExecAssignExprContext(estate, &node->ps);` to ` /* need an expression context to do the projection */ if (node->ps.ps_ExprContext == NULL) ExecAssignExprContext(estate, &node->ps); ` ? ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-02-28 09:16 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-12-08 18:06 [PATCH 2/2] Expose GUC flags in SQL function; retire ./check_guc Justin Pryzby <[email protected]> 2024-02-22 16:20 Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Tom Lane <[email protected]> 2024-02-23 00:12 ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Tom Lane <[email protected]> 2024-02-27 12:53 ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]> 2024-02-28 09:16 ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) jian he <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox