public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 09/23] vcregress: add alltaptests
6+ messages / 4 participants
[nested] [flat]

* [PATCH 09/23] vcregress: add alltaptests
@ 2022-02-25 23:00 Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 6+ messages in thread

From: Justin Pryzby @ 2022-02-25 23:00 UTC (permalink / raw)

https://www.postgresql.org/message-id/[email protected]
https://www.postgresql.org/message-id/flat/[email protected]

In passing, document taptest PROVE_FLAGS

ci-os-only: windows

See also:
d835dd6685246f0737ca42ab68242210681bb220
13d856e177e69083f543d6383eeda9e12ce3c55c
fed6df486dca1b9e53d3f560031b9a236c99f4bb
---
 .cirrus.yml                 | 14 ++++--------
 src/tools/msvc/vcregress.pl | 43 +++++++++++++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 2cabe6b7ec1..7b8ce6eadc3 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -401,9 +401,9 @@ task:
     # If tests hang forever, cirrus eventually times out. In that case log
     # output etc is not uploaded, making the problem hard to debug. Of course
     # tests internally should have shorter timeouts, but that's proven to not
-    # be sufficient. 15min currently is fast enough to finish individual test
+    # be sufficient. 25min currently is fast enough to finish individual test
     # "suites".
-    T_C: "\"C:/Program Files/Git/usr/bin/timeout.exe\" -v -k60s 15m"
+    T_C: "\"C:/Program Files/Git/usr/bin/timeout.exe\" -v -k60s 25m"
 
     # startcreate_script starts a postgres instance that we don't want to get
     # killed at the end of that script (it's stopped in stop_script). Can't
@@ -461,14 +461,8 @@ task:
   test_ssl_script: |
     set with_ssl=openssl
     %T_C% perl src/tools/msvc/vcregress.pl taptest ./src/test/ssl/
-  test_subscription_script: |
-    %T_C% perl src/tools/msvc/vcregress.pl taptest ./src/test/subscription/
-  test_authentication_script: |
-    %T_C% perl src/tools/msvc/vcregress.pl taptest ./src/test/authentication/
-  test_recovery_script: |
-    %T_C% perl src/tools/msvc/vcregress.pl recoverycheck
-  test_bin_script: |
-    %T_C% perl src/tools/msvc/vcregress.pl bincheck
+  test_tap_script: |
+    %T_C% perl src/tools/msvc/vcregress.pl alltaptests
   test_ecpg_script: |
     rem tries to build additional stuff
     vcvarsall x64
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 2d6ccd45419..6495f33e593 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -51,7 +51,7 @@ if (-e "src/tools/msvc/buildenv.pl")
 
 my $what = shift || "";
 if ($what =~
-	/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|bincheck|recoverycheck|taptest)$/i
+	/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|bincheck|recoverycheck|taptest|alltaptests)$/i
   )
 {
 	$what = uc $what;
@@ -109,6 +109,7 @@ my %command = (
 	BINCHECK       => \&bincheck,
 	RECOVERYCHECK  => \&recoverycheck,
 	UPGRADECHECK   => \&upgradecheck,     # no-op
+	ALLTAPTESTS    => \&alltaptests,
 	TAPTEST        => \&taptest,);
 
 my $proc = $command{$what};
@@ -296,6 +297,9 @@ sub tap_check
 	# add the module build dir as the second element in the PATH
 	$ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
 
+	print "============================================================\n";
+	print "Checking $dir: @args\n";
+
 	rmtree('tmp_check');
 	system(@args);
 	my $status = $? >> 8;
@@ -310,8 +314,7 @@ sub bincheck
 
 	my $mstat = 0;
 
-	# Find out all the existing TAP tests by looking for t/ directories
-	# in the tree.
+	# Find the TAP tests by looking for t/ directories
 	my @bin_dirs = glob("$topdir/src/bin/*");
 
 	# Process each test
@@ -326,6 +329,36 @@ sub bincheck
 	return;
 }
 
+sub alltaptests
+{
+	InstallTemp();
+
+	my $mstat = 0;
+
+	# Find out all the existing TAP tests by looking for t/ directories
+	# in the tree.
+	my @tap_dirs = ();
+	my @top_dir = ($topdir);
+	File::Find::find(
+		{   wanted => sub {
+			/^t\z/s
+			  && $File::Find::name !~ /\/(kerberos|ldap|ssl|ssl_passphrase_callback)\// # opt-in: warn about these?
+			  && push(@tap_dirs, $File::Find::name);
+			}
+		},
+		@top_dir);
+
+	# Process each test
+	foreach my $test_path (@tap_dirs)
+	{
+		my $dir = dirname($test_path);
+		my $status = tap_check($dir);
+		$mstat ||= $status;
+	}
+	exit $mstat if $mstat;
+	return;
+}
+
 sub taptest
 {
 	my $dir = shift;
@@ -676,6 +709,7 @@ sub usage
 	print STDERR
 	  "Usage: vcregress.pl <mode> [<arg>]\n\n",
 	  "Options for <mode>:\n",
+	  "  alltaptests    run all tap tests (except kerberos, ldap, ssl, ssl_passphrase_callback)\n",
 	  "  bincheck       run tests of utilities in src/bin/\n",
 	  "  check          deploy instance and run regression tests on it\n",
 	  "  contribcheck   run tests of modules in contrib/\n",
@@ -693,6 +727,7 @@ sub usage
 	  "\nOptions for <arg>: (used by contribcheck and modulescheck)\n",
 	  "  install        also run tests which require a new instance\n",
 	  "\nOption for <arg>: for taptest\n",
-	  "  TEST_DIR       (required) directory where tests reside\n";
+	  "  TEST_DIR       (required) directory where tests reside\n",
+	  "  PROVE_FLAGS    flags to pass to prove\n";
 	exit(1);
 }
-- 
2.17.1


--tKkaNMvYmhQvRCRK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0010-tmp-run-tap-tests-first.patch"



^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)
@ 2024-02-28 12:11 Dean Rasheed <[email protected]>
  2024-02-29 03:04 ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) jian he <[email protected]>
  0 siblings, 1 reply; 6+ messages in thread

From: Dean Rasheed @ 2024-02-28 12:11 UTC (permalink / raw)
  To: jian he <[email protected]>; +Cc: Tom Lane <[email protected]>; zwj <[email protected]>; pgsql-hackers <[email protected]>

On Wed, 28 Feb 2024 at 09:16, jian he <[email protected]> wrote:
>
> + 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,
> +
> EvalPlanQualStart, EvalPlanQualNext will switch the memory context to
> es_query_cxt.
> so the memory context switch here is not necessary?
>

Yes it is necessary. The EvalPlanQual mechanism switches to the
epqstate->recheckestate->es_query_cxt memory context, which is not the
same as the main query's estate->es_query_cxt (they're different
executor states). Most stuff allocated under EvalPlanQual() is
intended to be short-lived (just for the duration of that specific EPQ
check), whereas this stuff (the TupleDesc and Projection) is intended
to last for the duration of the main query, so that it can be reused
in later EPQ checks.

> 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);
> `
> ?

Possibly. More importantly, it should be doing a ResetExprContext() to
free any previous stuff before projecting the new row.

At this stage, this is just a rough draft patch. There are many things
like that and code comments that will need to be improved before it is
committable, but for now I'm more concerned with whether it actually
works, and if the approach it's taking is sane.

I've tried various things and I haven't been able to break it, but I'm
still nervous that I might be overlooking something, particularly in
relation to what might get added to the targetlist at various stages
during planning for different types of query.

Regards,
Dean






^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)
  2024-02-28 12:11 Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
@ 2024-02-29 03:04 ` jian he <[email protected]>
  2024-03-05 10:04   ` =?gb18030?B?u9i4tKO6IGJ1ZyByZXBvcnQ6IHNvbWUgaXNzdWVz?= =?gb18030?B?IGFib3V0IHBnXzE1X3N0YWJsZSg4ZmE0YTFhYzYx?= =?gb18030?B?MTg5ZWZmZmI4Yjg1MWVlNzdlMWJjODczNjBjNDQ1?= =?gb18030?B?KQ==?= =?gb18030?B?endq?= <[email protected]>
  0 siblings, 1 reply; 6+ messages in thread

From: jian he @ 2024-02-29 03:04 UTC (permalink / raw)
  To: Dean Rasheed <[email protected]>; +Cc: Tom Lane <[email protected]>; zwj <[email protected]>; pgsql-hackers <[email protected]>

On Wed, Feb 28, 2024 at 8:11 PM Dean Rasheed <[email protected]> wrote:
>
> On Wed, 28 Feb 2024 at 09:16, jian he <[email protected]> wrote:
> >
> > + 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,
> > +
> > EvalPlanQualStart, EvalPlanQualNext will switch the memory context to
> > es_query_cxt.
> > so the memory context switch here is not necessary?
> >
>
> Yes it is necessary. The EvalPlanQual mechanism switches to the
> epqstate->recheckestate->es_query_cxt memory context, which is not the
> same as the main query's estate->es_query_cxt (they're different
> executor states). Most stuff allocated under EvalPlanQual() is
> intended to be short-lived (just for the duration of that specific EPQ
> check), whereas this stuff (the TupleDesc and Projection) is intended
> to last for the duration of the main query, so that it can be reused
> in later EPQ checks.
>
sorry for the noise. I understand it now.

Another small question:
for the Append case, we can set/initialize it at create_append_plan,
all other elements are initialized there,
why we set it at set_append_references.
just wondering.






^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* =?gb18030?B?u9i4tKO6IGJ1ZyByZXBvcnQ6IHNvbWUgaXNzdWVz?= =?gb18030?B?IGFib3V0IHBnXzE1X3N0YWJsZSg4ZmE0YTFhYzYx?= =?gb18030?B?MTg5ZWZmZmI4Yjg1MWVlNzdlMWJjODczNjBjNDQ1?= =?gb18030?B?KQ==?=
  2024-02-28 12:11 Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
  2024-02-29 03:04 ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) jian he <[email protected]>
@ 2024-03-05 10:04   ` =?gb18030?B?endq?= <[email protected]>
  2024-03-05 13:55     ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
  0 siblings, 1 reply; 6+ messages in thread

From: =?gb18030?B?endq?= @ 2024-03-05 10:04 UTC (permalink / raw)
  To: =?gb18030?B?RGVhbiBSYXNoZWVk?= <[email protected]>; =?gb18030?B?amlhbiBoZQ==?= <[email protected]>; =?gb18030?B?VG9tIExhbmU=?= <[email protected]>; +Cc: =?gb18030?B?cGdzcWwtaGFja2Vycw==?= <[email protected]>

Hi,hackers
&nbsp;
&nbsp;I may have discovered another issue in the concurrency scenario of merge,&nbsp; and I am currently not sure if this new issue is related to the previous one.&nbsp;
&nbsp;It seems that it may also be an issue with the EPQ mechanism in the merge scenario?&nbsp;
&nbsp;I will provide this test case, hoping it will be helpful for you to fix related issues in the future.



&nbsp;&nbsp;&nbsp; DROP TABLE IF EXISTS src1, tgt;
&nbsp;&nbsp;&nbsp; CREATE TABLE src1 (a int, b text);
&nbsp;&nbsp;&nbsp; CREATE TABLE tgt (a int, b text);
&nbsp;&nbsp;&nbsp; INSERT INTO src1 SELECT x, 'Src1 '||x FROM generate_series(1, 3) g(x);
&nbsp;&nbsp;&nbsp; INSERT INTO tgt SELECT x, 'Tgt '||x FROM generate_series(1, 6, 2) g(x);
&nbsp;&nbsp;&nbsp; insert into src1 values(3,'src1 33');



&nbsp;If I only execute merge , I will get the following error:
&nbsp;&nbsp;&nbsp; merge into tgt a using src1 c on&nbsp; a.a = c.a when matched then update set b = c.b when not matched then insert (a,b) values(c.a,c.b);&nbsp; -- excute fail
&nbsp;&nbsp;&nbsp; ERROR:&nbsp; MERGE command cannot affect row a second time
&nbsp;&nbsp;&nbsp; HIINT:&nbsp; Ensure that not more than one source row matches any one target row.



&nbsp;But when I execute the update and merge concurrently, I will get the following result set.

&nbsp; --session1
&nbsp;&nbsp;&nbsp; begin;

&nbsp;&nbsp;&nbsp; update tgt set b = 'tgt333' where a =3;

&nbsp; --session2
&nbsp;&nbsp;&nbsp; merge into tgt a using src1 c on&nbsp; a.a = c.a when matched then update set  b = c.b when not matched then insert (a,b) values(c.a,c.b);&nbsp; -- excute success
&nbsp; --session1
&nbsp;&nbsp;&nbsp; commit;
&nbsp;&nbsp;&nbsp; select * from tgt;
&nbsp;&nbsp; &nbsp; a |&nbsp;&nbsp;&nbsp; b&nbsp;&nbsp; &nbsp;
&nbsp;&nbsp;&nbsp; ---+---------
&nbsp;&nbsp;&nbsp;&nbsp; 5 | Tgt 5
&nbsp;&nbsp;&nbsp;&nbsp; 1 | Src1 1
&nbsp;&nbsp;&nbsp;&nbsp; 2 | Src1 2
&nbsp;&nbsp;&nbsp;&nbsp; 3 | Src1 3
&nbsp;&nbsp;&nbsp;&nbsp; 3 | src1 33

&nbsp; I think even if the tuple with id:3 is udpated, merge should still be able to retrieve new tuples with id:3,&nbsp; and&nbsp;report the same error as above?


Regards,
wenjiang zhang


------------------&nbsp;原始邮件&nbsp;------------------
发件人:                                                                                                                        "jian he"                                                                                    <[email protected]&gt;;
发送时间:&nbsp;2024年2月29日(星期四) 中午11:04
收件人:&nbsp;"Dean Rasheed"<[email protected]&gt;;
抄送:&nbsp;"Tom Lane"<[email protected]&gt;;"zwj"<[email protected]&gt;;"pgsql-hackers"<[email protected]&gt;;
主题:&nbsp;Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)



On Wed, Feb 28, 2024 at 8:11 PM Dean Rasheed <[email protected]&gt; wrote:
&gt;
&gt; On Wed, 28 Feb 2024 at 09:16, jian he <[email protected]&gt; wrote:
&gt; &gt;
&gt; &gt; + oldcontext = MemoryContextSwitchTo(estate-&gt;es_query_cxt);
&gt; &gt; +
&gt; &gt; + node-&gt;as_epq_tupdesc = lookup_rowtype_tupdesc_copy(tupType, tupTypmod);
&gt; &gt; +
&gt; &gt; + ExecAssignExprContext(estate, &amp;node-&gt;ps);
&gt; &gt; +
&gt; &gt; + node-&gt;ps.ps_ProjInfo =
&gt; &gt; + ExecBuildProjectionInfo(castNode(Append, node-&gt;ps.plan)-&gt;epq_targetlist,
&gt; &gt; +
&gt; &gt; EvalPlanQualStart, EvalPlanQualNext will switch the memory context to
&gt; &gt; es_query_cxt.
&gt; &gt; so the memory context switch here is not necessary?
&gt; &gt;
&gt;
&gt; Yes it is necessary. The EvalPlanQual mechanism switches to the
&gt; epqstate-&gt;recheckestate-&gt;es_query_cxt memory context, which is not the
&gt; same as the main query's estate-&gt;es_query_cxt (they're different
&gt; executor states). Most stuff allocated under EvalPlanQual() is
&gt; intended to be short-lived (just for the duration of that specific EPQ
&gt; check), whereas this stuff (the TupleDesc and Projection) is intended
&gt; to last for the duration of the main query, so that it can be reused
&gt; in later EPQ checks.
&gt;
sorry for the noise. I understand it now.

Another small question:
for the Append case, we can set/initialize it at create_append_plan,
all other elements are initialized there,
why we set it at set_append_references.
just wondering.

^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)
  2024-02-28 12:11 Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
  2024-02-29 03:04 ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) jian he <[email protected]>
  2024-03-05 10:04   ` =?gb18030?B?u9i4tKO6IGJ1ZyByZXBvcnQ6IHNvbWUgaXNzdWVz?= =?gb18030?B?IGFib3V0IHBnXzE1X3N0YWJsZSg4ZmE0YTFhYzYx?= =?gb18030?B?MTg5ZWZmZmI4Yjg1MWVlNzdlMWJjODczNjBjNDQ1?= =?gb18030?B?KQ==?= =?gb18030?B?endq?= <[email protected]>
@ 2024-03-05 13:55     ` Dean Rasheed <[email protected]>
  2024-03-07 10:20       ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
  0 siblings, 1 reply; 6+ messages in thread

From: Dean Rasheed @ 2024-03-05 13:55 UTC (permalink / raw)
  To: zwj <[email protected]>; +Cc: jian he <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers <[email protected]>; Alvaro Herrera <[email protected]>

[cc'ing Alvaro]

On Tue, 5 Mar 2024 at 10:04, zwj <[email protected]> wrote:
>
>  If I only execute merge , I will get the following error:
>     merge into tgt a using src1 c on  a.a = c.a when matched then update set b = c.b when not matched then insert (a,b) values(c.a,c.b);  -- excute fail
>     ERROR:  MERGE command cannot affect row a second time
>     HIINT:  Ensure that not more than one source row matches any one target row.
>
>  But when I execute the update and merge concurrently, I will get the following result set.
>

Yes, this should still produce that error, even after a concurrent update.

In the first example without a concurrent update, when we reach the
tgt.a = 3 row the second time, ExecUpdateAct() returns TM_SelfModified
and we do this:

    case TM_SelfModified:

        /*
         * The SQL standard disallows this for MERGE.
         */
        if (TransactionIdIsCurrentTransactionId(context->tmfd.xmax))
            ereport(ERROR,
                    (errcode(ERRCODE_CARDINALITY_VIOLATION),
            /* translator: %s is a SQL command name */
                     errmsg("%s command cannot affect row a second time",
                            "MERGE"),
                     errhint("Ensure that not more than one source row
matches any one target row.")));
        /* This shouldn't happen */
        elog(ERROR, "attempted to update or delete invisible tuple");
        break;

whereas in the second case, after a concurrent update, ExecUpdateAct()
returns TM_Updated, we attempt to lock the tuple prior to running EPQ,
and table_tuple_lock() returns TM_SelfModified, which does this:

            case TM_SelfModified:

                /*
                 * This can be reached when following an update
                 * chain from a tuple updated by another session,
                 * reaching a tuple that was already updated in
                 * this transaction. If previously modified by
                 * this command, ignore the redundant update,
                 * otherwise error out.
                 *
                 * See also response to TM_SelfModified in
                 * ExecUpdate().
                 */
                if (context->tmfd.cmax != estate->es_output_cid)
                    ereport(ERROR,
                            (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
                             errmsg("tuple to be updated or deleted
was already modified by an operation triggered by the current
command"),
                             errhint("Consider using an AFTER trigger
instead of a BEFORE trigger to propagate changes to other rows.")));
                return false;

My initial reaction is that neither of those blocks of code is
entirely correct, and that they should both be doing both of those
checks. I.e., something like the attached (which probably needs some
additional test cases).

Regards,
Dean


Attachments:

  [text/x-patch] merge-self-modified-tuple.patch (3.6K, ../../CAEZATCVCFDWQXyfBgcO8HxoH22aV1kKbBnJhRY7+wWDLRCMV2w@mail.gmail.com/2-merge-self-modified-tuple.patch)
  download | inline diff:
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
new file mode 100644
index fcb6133..9351fbc
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -3001,8 +3001,29 @@ lmerge_matched:
 			case TM_SelfModified:
 
 				/*
-				 * The SQL standard disallows this for MERGE.
+				 * The target tuple was already updated or deleted by the
+				 * current command, or by a later command in the current
+				 * transaction.  The former case is explicitly disallowed by
+				 * the SQL standard for MERGE, which insists that the MERGE
+				 * join condition should not join a target row to more than
+				 * one source row.
+				 *
+				 * The latter case arises if the tuple is modified by a
+				 * command in a BEFORE trigger, or perhaps by a command in a
+				 * volatile function used in the query.  In such situations we
+				 * should not ignore the MERGE action, but it is equally
+				 * unsafe to proceed.  We don't want to discard the original
+				 * MERGE action while keeping the triggered actions based on
+				 * it; and it would be no better to allow the original MERGE
+				 * action while discarding the updates that it triggered.  So
+				 * throwing an error is the only safe course.
 				 */
+				if (context->tmfd.cmax != estate->es_output_cid)
+					ereport(ERROR,
+							(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
+							 errmsg("tuple to be updated or deleted was already modified by an operation triggered by the current command"),
+							 errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.")));
+
 				if (TransactionIdIsCurrentTransactionId(context->tmfd.xmax))
 					ereport(ERROR,
 							(errcode(ERRCODE_CARDINALITY_VIOLATION),
@@ -3010,6 +3031,7 @@ lmerge_matched:
 							 errmsg("%s command cannot affect row a second time",
 									"MERGE"),
 							 errhint("Ensure that not more than one source row matches any one target row.")));
+
 				/* This shouldn't happen */
 				elog(ERROR, "attempted to update or delete invisible tuple");
 				break;
@@ -3118,19 +3140,27 @@ lmerge_matched:
 							/*
 							 * This can be reached when following an update
 							 * chain from a tuple updated by another session,
-							 * reaching a tuple that was already updated in
-							 * this transaction. If previously modified by
-							 * this command, ignore the redundant update,
-							 * otherwise error out.
-							 *
-							 * See also response to TM_SelfModified in
-							 * ExecUpdate().
+							 * reaching a tuple that was already updated or
+							 * deleted by the current command, or by a later
+							 * command in the current transaction. As above,
+							 * this should always be treated as an error.
 							 */
 							if (context->tmfd.cmax != estate->es_output_cid)
 								ereport(ERROR,
 										(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
 										 errmsg("tuple to be updated or deleted was already modified by an operation triggered by the current command"),
 										 errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.")));
+
+							if (TransactionIdIsCurrentTransactionId(context->tmfd.xmax))
+								ereport(ERROR,
+										(errcode(ERRCODE_CARDINALITY_VIOLATION),
+								/* translator: %s is a SQL command name */
+										 errmsg("%s command cannot affect row a second time",
+												"MERGE"),
+										 errhint("Ensure that not more than one source row matches any one target row.")));
+
+							/* This shouldn't happen */
+							elog(ERROR, "attempted to update or delete invisible tuple");
 							return false;
 
 						default:


^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)
  2024-02-28 12:11 Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
  2024-02-29 03:04 ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) jian he <[email protected]>
  2024-03-05 10:04   ` =?gb18030?B?u9i4tKO6IGJ1ZyByZXBvcnQ6IHNvbWUgaXNzdWVz?= =?gb18030?B?IGFib3V0IHBnXzE1X3N0YWJsZSg4ZmE0YTFhYzYx?= =?gb18030?B?MTg5ZWZmZmI4Yjg1MWVlNzdlMWJjODczNjBjNDQ1?= =?gb18030?B?KQ==?= =?gb18030?B?endq?= <[email protected]>
  2024-03-05 13:55     ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
@ 2024-03-07 10:20       ` Dean Rasheed <[email protected]>
  0 siblings, 0 replies; 6+ messages in thread

From: Dean Rasheed @ 2024-03-07 10:20 UTC (permalink / raw)
  To: zwj <[email protected]>; +Cc: jian he <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers <[email protected]>; Alvaro Herrera <[email protected]>

On Tue, 5 Mar 2024 at 13:55, Dean Rasheed <[email protected]> wrote:
>
> >  If I only execute merge , I will get the following error:
> >     merge into tgt a using src1 c on  a.a = c.a when matched then update set b = c.b when not matched then insert (a,b) values(c.a,c.b);  -- excute fail
> >     ERROR:  MERGE command cannot affect row a second time
> >     HIINT:  Ensure that not more than one source row matches any one target row.
> >
> >  But when I execute the update and merge concurrently, I will get the following result set.
>
> Yes, this should still produce that error, even after a concurrent update.
>
> My initial reaction is that neither of those blocks of code is
> entirely correct, and that they should both be doing both of those
> checks. I.e., something like the attached (which probably needs some
> additional test cases).
>

OK, I've pushed and back-patched that fix for this issue, after adding
some tests (nice catch, by the way!).

That wasn't related to the original issue though, so the problem with
UNION ALL still remains to be fixed. The patch from [1] looks
promising (for HEAD at least), but it really needs more pairs of eyes
on it (bearing in mind that it's just a rough proof-of-concept patch
at this stage).

[1] https://www.postgresql.org/message-id/CAEZATCVa-mgPuOdgd8%2BYVgOJ4wgJuhT2mJznbj_tmsGAP8hHJw%40mail.g...

Regards,
Dean






^ permalink  raw  reply  [nested|flat] 6+ messages in thread


end of thread, other threads:[~2024-03-07 10:20 UTC | newest]

Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-02-25 23:00 [PATCH 09/23] vcregress: add alltaptests Justin Pryzby <[email protected]>
2024-02-28 12:11 Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
2024-02-29 03:04 ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) jian he <[email protected]>
2024-03-05 10:04   ` =?gb18030?B?u9i4tKO6IGJ1ZyByZXBvcnQ6IHNvbWUgaXNzdWVz?= =?gb18030?B?IGFib3V0IHBnXzE1X3N0YWJsZSg4ZmE0YTFhYzYx?= =?gb18030?B?MTg5ZWZmZmI4Yjg1MWVlNzdlMWJjODczNjBjNDQ1?= =?gb18030?B?KQ==?= =?gb18030?B?endq?= <[email protected]>
2024-03-05 13:55     ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[email protected]>
2024-03-07 10:20       ` Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445) Dean Rasheed <[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