MIME-Version: 1.0
References: 
 <CAO6_Xqroaof=Gu_NCg4bZfpdgwOjNW9fg753cuuts-FBnv9UGw@mail.gmail.com>
 <a96682c3-9505-456f-b5c7-1b9d1e6303b4@manitou-mail.org>
 <Z8kilUVjYAHko17g@paquier.xyz>
 <ebf6ce77-b180-4d6b-8eab-71f641499ddf@postgrespro.ru>
 <Z__YOQJqBJYhezm2@paquier.xyz>
 <aAXkJIOildLUA7vQ@paquier.xyz> <aAbdfKU2ZWeuAnIG@paquier.xyz>
In-Reply-To: <aAbdfKU2ZWeuAnIG@paquier.xyz>
From: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Date: Tue, 22 Apr 2025 14:37:19 +0200
Message-ID: 
 <CAO6_XqoQaZVHEw1dFgvOxwectqsNo4QE1tQ6rLD-YUzFpT_+3g@mail.gmail.com>
Subject: Re: Add Pipelining support in psql
To: Michael Paquier <michael@paquier.xyz>
Cc: "a.kozhemyakin" <a.kozhemyakin@postgrespro.ru>,
 Daniel Verite <daniel@manitou-mail.org>,
	Jelte Fennema-Nio <postgres@jeltef.nl>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000008d995306335d3f46"
Archived-At: 
 <https://www.postgresql.org/message-id/CAO6_XqoQaZVHEw1dFgvOxwectqsNo4QE1tQ6rLD-YUzFpT_%2B3g%40mail.gmail.com>
Precedence: bulk

--0000000000008d995306335d3f46
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 22, 2025 at 2:06=E2=80=AFAM Michael Paquier <michael@paquier.xy=
z> wrote:
> I am wondering if we could not be smarter with the handling of
> the counters, but I really doubt that there is much more we can do
> under a PGRES_FATAL_ERROR.

One thing that bothers me is that the reported error is silently
discarded within discardAbortedPipelineResults.

psql -f bug_assertion.sql
psql:bug_assertion.sql:7: ERROR:  unexpected message type 0x50 during
COPY from stdin
CONTEXT:  COPY psql_pipeline, line 1
psql:bug_assertion.sql:7: Pipeline aborted, command did not run

This should ideally report the "FATAL:  terminating connection because
protocol synchronization was lost" sent by the backend process.

Also, we still have a triggered assertion failure with the following:
CREATE TABLE psql_pipeline(a text);
\startpipeline
COPY psql_pipeline FROM STDIN;
SELECT 'val1';
\syncpipeline
\endpipeline
...
Assertion failed: (pset.piped_syncs =3D=3D 0), function
ExecQueryAndProcessResults, file common.c, line 2153.

A possible alternative could be to abort discardAbortedPipelineResults
when we encounter a res !=3D NULL + FATAL error and let the outer loop
handle it. As you said, the pipeline flow is borked so there's not
much to salvage. The outer loop would read and print all error
messages until the closed connection is detected. Then,
CheckConnection will reset the connection which will reset the
pipeline state.

While testing this change, I was initially looking for the "FATAL:
terminating connection because protocol synchronization was lost"
message in the tests. However, this was failing on Windows[1] as the
FATAL message wasn't reported on stderr. I'm not sure why yet.

[1]: https://cirrus-ci.com/task/5051031505076224?logs=3Dcheck_world#L240-L2=
46

--0000000000008d995306335d3f46
Content-Type: application/octet-stream;
	name="v02-0001-PATCH-psql-Fix-assertion-failure-with-pipeline-m.patch"
Content-Disposition: attachment;
	filename="v02-0001-PATCH-psql-Fix-assertion-failure-with-pipeline-m.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_m9shgbt80>
X-Attachment-Id: f_m9shgbt80

RnJvbSA1ZDM3ZjI2MTZjODJjNjUyNWQ2NTYxNDlkMzgzZWYwMWE2ZDc1MThjIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBNaWNoYWVsIFBhcXVpZXIgPG1pY2hhZWxAcGFxdWllci54eXo+
CkRhdGU6IE1vbiwgMjEgQXByIDIwMjUgMTU6MTc6NDMgKzA5MDAKU3ViamVjdDogW1BBVENIXSBw
c3FsOiBGaXggYXNzZXJ0aW9uIGZhaWx1cmUgd2l0aCBwaXBlbGluZSBtb2RlCgotLS0KIHNyYy9i
aW4vcHNxbC9jb21tb24uYyAgICAgICB8IDE3ICsrKysrKysrKysrKwogc3JjL2Jpbi9wc3FsL3Qv
MDAxX2Jhc2ljLnBsIHwgNTUgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0tLQog
MiBmaWxlcyBjaGFuZ2VkLCA2OCBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQoKZGlmZiAt
LWdpdCBhL3NyYy9iaW4vcHNxbC9jb21tb24uYyBiL3NyYy9iaW4vcHNxbC9jb21tb24uYwppbmRl
eCAyMWQ2NjBhODk2MS4uMGFhYjAyZWUzMmUgMTAwNjQ0Ci0tLSBhL3NyYy9iaW4vcHNxbC9jb21t
b24uYworKysgYi9zcmMvYmluL3BzcWwvY29tbW9uLmMKQEAgLTE0NzgsNiArMTQ3OCwyMyBAQCBk
aXNjYXJkQWJvcnRlZFBpcGVsaW5lUmVzdWx0cyh2b2lkKQogCQkJICovCiAJCQlyZXR1cm4gcmVz
OwogCQl9CisJCWVsc2UgaWYgKHJlcyAhPSBOVUxMICYmIHJlc3VsdF9zdGF0dXMgPT0gUEdSRVNf
RkFUQUxfRVJST1IpCisJCXsKKwkJCS8qCisJCQkgKiBXZSBoYXZlIGEgZmF0YWwgZXJyb3Igc2Vu
dCBieSB0aGUgYmFja2VuZCBhbmQgd2UgY2FuJ3QgcmVjb3ZlcgorCQkJICogZnJvbSB0aGlzIHN0
YXRlLiBJbnN0ZWFkLCByZXR1cm4gdGhlIGxhc3QgZmF0YWwgZXJyb3IgYW5kIGxldAorCQkJICog
dGhlIG91dGVyIGxvb3AgaGFuZGxlIGl0LgorCQkJICovCisJCQlQR3Jlc3VsdCAgICpmYXRhbF9y
ZXMgUEdfVVNFRF9GT1JfQVNTRVJUU19PTkxZOworCisJCQkvKgorCQkJICogRmV0Y2ggcmVzdWx0
IHRvIGNvbnN1bWUgdGhlIGVuZCBvZiB0aGUgY3VycmVudCBxdWVyeSBiZWluZworCQkJICogcHJv
Y2Vzc2VkLgorCQkJICovCisJCQlmYXRhbF9yZXMgPSBQUWdldFJlc3VsdChwc2V0LmRiKTsKKwkJ
CUFzc2VydChmYXRhbF9yZXMgPT0gTlVMTCk7CisJCQlyZXR1cm4gcmVzOworCQl9CiAJCWVsc2Ug
aWYgKHJlcyA9PSBOVUxMKQogCQl7CiAJCQkvKiBBIHF1ZXJ5IHdhcyBwcm9jZXNzZWQsIGRlY3Jl
bWVudCB0aGUgY291bnRlcnMgKi8KZGlmZiAtLWdpdCBhL3NyYy9iaW4vcHNxbC90LzAwMV9iYXNp
Yy5wbCBiL3NyYy9iaW4vcHNxbC90LzAwMV9iYXNpYy5wbAppbmRleCA3MzljYjQzOTcwOC4uOGQy
NThjMDBjNWUgMTAwNjQ0Ci0tLSBhL3NyYy9iaW4vcHNxbC90LzAwMV9iYXNpYy5wbAorKysgYi9z
cmMvYmluL3BzcWwvdC8wMDFfYmFzaWMucGwKQEAgLTM0LDExICszNCwxMyBAQCBzdWIgcHNxbF9m
YWlsc19saWtlCiB7CiAJbG9jYWwgJFRlc3Q6OkJ1aWxkZXI6OkxldmVsID0gJFRlc3Q6OkJ1aWxk
ZXI6OkxldmVsICsgMTsKIAotCW15ICgkbm9kZSwgJHNxbCwgJGV4cGVjdGVkX3N0ZGVyciwgJHRl
c3RfbmFtZSkgPSBAXzsKKwlteSAoJG5vZGUsICRzcWwsICRleHBlY3RlZF9zdGRlcnIsICR0ZXN0
X25hbWUsICRyZXBsaWNhdGlvbikgPSBAXzsKKworCSMgVXNlIHRoZSBjb250ZXh0IG9mIGEgV0FM
IHNlbmRlciwgaWYgcmVxdWVzdGVkIGJ5IHRoZSBjYWxsZXIuCisJJHJlcGxpY2F0aW9uID0gJycg
dW5sZXNzIGRlZmluZWQoJHJlcGxpY2F0aW9uKTsKIAotCSMgVXNlIHRoZSBjb250ZXh0IG9mIGEg
V0FMIHNlbmRlciwgc29tZSBvZiB0aGUgdGVzdHMgcmVseSBvbiB0aGF0LgogCW15ICgkcmV0LCAk
c3Rkb3V0LCAkc3RkZXJyKSA9Ci0JICAkbm9kZS0+cHNxbCgncG9zdGdyZXMnLCAkc3FsLCByZXBs
aWNhdGlvbiA9PiAnZGF0YWJhc2UnKTsKKwkgICRub2RlLT5wc3FsKCdwb3N0Z3JlcycsICRzcWws
IHJlcGxpY2F0aW9uID0+ICRyZXBsaWNhdGlvbik7CiAKIAlpc250KCRyZXQsIDAsICIkdGVzdF9u
YW1lOiBleGl0IGNvZGUgbm90IDAiKTsKIAlsaWtlKCRzdGRlcnIsICRleHBlY3RlZF9zdGRlcnIs
ICIkdGVzdF9uYW1lOiBtYXRjaGVzIik7CkBAIC03OSw3ICs4MSw4IEBAIHBzcWxfZmFpbHNfbGlr
ZSgKIAkkbm9kZSwKIAknU1RBUlRfUkVQTElDQVRJT04gMC8wJywKIAlxci91bmV4cGVjdGVkIFBR
cmVzdWx0U3RhdHVzOiA4JC8sCi0JJ2hhbmRsaW5nIG9mIHVuZXhwZWN0ZWQgUFFyZXN1bHRTdGF0
dXMnKTsKKwknaGFuZGxpbmcgb2YgdW5leHBlY3RlZCBQUXJlc3VsdFN0YXR1cycsCisJJ2RhdGFi
YXNlJyk7CiAKICMgdGVzdCBcdGltaW5nCiBwc3FsX2xpa2UoCkBAIC00ODEsNCArNDg0LDQ4IEBA
IHBzcWxfbGlrZSgkbm9kZSwgImNvcHkgKHZhbHVlcyAoJ2ZvbycpLCgnYmFyJykpIHRvIHN0ZG91
dCBcXGcgfCAkcGlwZV9jbWQiLAogbXkgJGM0ID0gc2x1cnBfZmlsZSgkZ19maWxlKTsKIGxpa2Uo
JGM0LCBxci9mb28uKmJhci9zKTsKIAorIyBUZXN0cyB3aXRoIHBpcGVsaW5lcy4gIFRoZXNlIHRy
aWdnZXIgRkFUQUwgZmFpbHVyZXMgaW4gdGhlIGJhY2tlbmQsCisjIHNvIHRoZXkgY2Fubm90IGJl
IHRlc3RlZCB0aHJvdWdoIHRoZSBTUUwgcmVncmVzc2lvbiB0ZXN0cy4KKyRub2RlLT5zYWZlX3Bz
cWwoJ3Bvc3RncmVzJywgJ0NSRUFURSBUQUJMRSBwc3FsX3BpcGVsaW5lKCknKTsKK3BzcWxfZmFp
bHNfbGlrZSgKKwkkbm9kZSwKKwlxcXtcXHN0YXJ0cGlwZWxpbmUKK0NPUFkgcHNxbF9waXBlbGlu
ZSBGUk9NIFNURElOOworU0VMRUNUICd2YWwxJzsKK1xcc3luY3BpcGVsaW5lCitcXGdldHJlc3Vs
dHMKK1xcZW5kcGlwZWxpbmV9LAorCXFyL3NlcnZlciBjbG9zZWQgdGhlIGNvbm5lY3Rpb24gdW5l
eHBlY3RlZGx5LywKKwknaGFuZGxpbmcgb2YgcHJvdG9jb2wgc3luY2hyb25pemF0aW9uIGxvc3Mg
d2l0aCBwaXBlbGluZXMnKTsKK3BzcWxfZmFpbHNfbGlrZSgKKwkkbm9kZSwKKwlxcXtcXHN0YXJ0
cGlwZWxpbmUKK0NPUFkgcHNxbF9waXBlbGluZSBGUk9NIFNURElOIFxcYmluZCBcXHNlbmRwaXBl
bGluZQorU0VMRUNUICd2YWwxJyBcXGJpbmQgXFxzZW5kcGlwZWxpbmUKK1xcc3luY3BpcGVsaW5l
CitcXGdldHJlc3VsdHMKK1xcZW5kcGlwZWxpbmV9LAorCXFyL3NlcnZlciBjbG9zZWQgdGhlIGNv
bm5lY3Rpb24gdW5leHBlY3RlZGx5LywKKwknaGFuZGxpbmcgb2YgcHJvdG9jb2wgc3luY2hyb25p
emF0aW9uIGxvc3Mgd2l0aCBwaXBlbGluZXMnKTsKKworIyBUaGlzIHRpbWUsIHRlc3Qgd2l0aG91
dCB0aGUgXGdldHJlc3VsdHMKK3BzcWxfZmFpbHNfbGlrZSgKKwkkbm9kZSwKKwlxcXtcXHN0YXJ0
cGlwZWxpbmUKK0NPUFkgcHNxbF9waXBlbGluZSBGUk9NIFNURElOOworU0VMRUNUICd2YWwxJzsK
K1xcc3luY3BpcGVsaW5lCitcXGVuZHBpcGVsaW5lfSwKKwlxci9zZXJ2ZXIgY2xvc2VkIHRoZSBj
b25uZWN0aW9uIHVuZXhwZWN0ZWRseS8sCisJJ2hhbmRsaW5nIG9mIHByb3RvY29sIHN5bmNocm9u
aXphdGlvbiBsb3NzIHdpdGggcGlwZWxpbmVzJyk7Citwc3FsX2ZhaWxzX2xpa2UoCisJJG5vZGUs
CisJcXF7XFxzdGFydHBpcGVsaW5lCitDT1BZIHBzcWxfcGlwZWxpbmUgRlJPTSBTVERJTiBcXGJp
bmQgXFxzZW5kcGlwZWxpbmUKK1NFTEVDVCAndmFsMScgXFxiaW5kIFxcc2VuZHBpcGVsaW5lCitc
XHN5bmNwaXBlbGluZQorXFxlbmRwaXBlbGluZX0sCisJcXIvc2VydmVyIGNsb3NlZCB0aGUgY29u
bmVjdGlvbiB1bmV4cGVjdGVkbHkvLAorCSdoYW5kbGluZyBvZiBwcm90b2NvbCBzeW5jaHJvbml6
YXRpb24gbG9zcyB3aXRoIHBpcGVsaW5lcycpOworCiBkb25lX3Rlc3RpbmcoKTsKLS0gCjIuMzku
NSAoQXBwbGUgR2l0LTE1NCkKCg==
--0000000000008d995306335d3f46--