Since commit 2549f0661, we reject an identifier immediately following
a numeric literal (without separating whitespace), because that risks
ambiguity with hex/octal/binary integers. However, that patch used
token patterns like "{integer}{ident_start}", which is problematic
because {ident_start} matches only a single byte. If the first
character after the integer is a multibyte character, this ends up
with flex reporting an error message that includes a partial multibyte
character. That can cause assorted bad-encoding problems downstream,
both in the report to the client and in the postmaster log file.
To fix, use {identifier} not {ident_start} in the "junk" token
patterns, so that they will match complete multibyte characters.
This seems generally better user experience quite aside from the
encoding problem: for "123abc" the error message will now say that
the error appeared at or near "123abc" instead of "123a".
While at it, add some commentary about why these patterns exist
and how they work.
Report and patch by Karina Litskevich; review by Pavel Borisov.
Back-patch to v15 where the problem came in.
Discussion: https://postgr.es/m/CACiT8iZ_diop=0zJ7zuY3BXegJpkKK1Av-PU7xh0EDYHsa5+=g@mail.gmail.com
Commit 0b9466fce added a dependency on fe_memutils' pnstrdup() inside
informix.c. This adds an exit() path in a library, which we don't
want. (Unlike libpq, the ecpg libraries don't have an automated check
for that, but it makes sense to keep them to a similar standard.) The
ecpg code can already handle failure results from the *strdup() call
by itself.
Author: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://www.postgresql.org/message-id/CAOYmi+=pg=W5L1h=3MEP_EB24jaBu2FyATrLXqQHGe7cpuvwyg@mail.gmail.com
Commit 453c468737 introduced a use of strerror() into libpq, but that
is not thread-safe. Fix by using strerror_r() instead.
In passing, update some of the code comments added by 453c468737, as
we have learned more about the reason for the change in OpenSSL that
started this.
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: Discussion: https://postgr.es/m/b6fb018b-f05c-4afd-abd3-318c649faf18@highgo.ca
Coverity complains that ECPGdebug is accessing debugstream without
holding debug_mutex, which is a fair complaint: we should take
debug_mutex while changing the settings ecpg_log looks at.
In some branches it also complains about unlocked use of simple_debug.
I think it's intentional and safe to have a quick unlocked check of
simple_debug at the start of ecpg_log, since that early exit will
always be taken in non-debug cases. But we should recheck
simple_debug after acquiring the mutex. In the worst case, calling
ECPGdebug concurrently with ecpg_log in another thread could result
in a null-pointer dereference due to debugstream transiently being
NULL while simple_debug isn't 0.
This is largely hypothetical, since it's unlikely anybody uses
ECPGdebug() at all in the field, and our own regression tests
don't seem to be hitting the theoretical race conditions either.
Still, if we're going to the trouble of having mutexes here, we ought
to be using them in a way that's actually safe not just almost safe.
Hence, back-patch to all supported branches.
The code associated with EXEC SQL DEFINE was unreadable and full of
bugs, notably:
* It'd attempt to free a non-malloced string if the ecpg program
tries to redefine a macro that was defined on the command line.
* Possible memory stomp if user writes "-D=foo".
* Undef'ing or redefining a macro defined on the command line would
change the state visible to the next file, when multiple files are
specified on the command line. (While possibly that could have been
an intentional choice, the code clearly intends to revert to the
original macro state; it's just failing to consider this interaction.)
* Missing "break" in defining a new macro meant that redefinition
of an existing name would cause an extra entry to be added to the
definition list. While not immediately harmful, a subsequent undef
would result in the prior entry becoming visible again.
* The interactions with input buffering are subtle and were entirely
undocumented.
It's not that surprising that we hadn't noticed these bugs,
because there was no test coverage at all of either the -D
command line switch or multiple input files. This patch adds
such coverage (in a rather hacky way I guess).
In addition to the code bugs, the user documentation was confused
about whether the -D switch defines a C macro or an ecpg one, and
it failed to mention that you can write "-Dsymbol=value".
These problems are old, so back-patch to all supported branches.
Discussion: https://postgr.es/m/998011.1713217712@sss.pgh.pa.us
ecpg wants to emit a warning if it parses a SQL construct that the
backend can parse but will immediately throw a FEATURE_NOT_SUPPORTED
error for. The way it was testing for this was to see if the string
ERRCODE_FEATURE_NOT_SUPPORTED appeared anywhere in the gram.y code.
This is, of course, not nearly good enough, as there are plenty of
rules in gram.y that throw that error only conditionally. There was
a hack dating to 2008 to suppress the warning in one rule that
doesn't even exist anymore, but nothing for other cases we've created
since then. End result was that you could get "unsupported feature
will be passed to server" warnings while compiling perfectly good SQL
code in ecpg. Somehow we'd not heard complaints about this, but
it was exposed by the recent addition of an ecpg test for a SQL/JSON
construct.
To fix, suppress the warning if the rule contains any "if" statement.
Manual comparison of gram.y with the generated preproc.y file shows
that the warning is now emitted only in rules where it's sensible.
This problem has existed for a long time, so back-patch to all
supported branches.
Discussion: https://postgr.es/m/603615.1712245382@sss.pgh.pa.us
If we are building with openssl but USE_SSL_ENGINE didn't get set,
initialize_SSL's variable "pkey" is declared but used nowhere.
Apparently this combination hasn't been exercised in the buildfarm
before now, because I've not seen this warning before, even though
the code has been like this a long time. Move the declaration
to silence the warning (and remove its useless initialization).
Per buildfarm member sawshark. Back-patch to all supported branches.
In OpenSSL 3.0.0 and later, ERR_reason_error_string randomly refuses
to provide a string for error codes representing system errno values
(e.g., "No such file or directory"). There is a poorly-documented way
to extract the errno from the SSL error code in this case, so do that
and apply strerror, rather than falling back to reporting the error
code's numeric value as we were previously doing.
Problem reported by David Zhang, although this is not his proposed
patch; it's instead based on a suggestion from Heikki Linnakangas.
Back-patch to all supported branches, since any of them are likely
to be used with recent OpenSSL.
Discussion: https://postgr.es/m/b6fb018b-f05c-4afd-abd3-318c649faf18@highgo.ca
intoasc(), a wrapper for PGTYPESinterval_to_asc that converts an
interval to its textual representation, used a plain memcpy() when
copying its result. This could miss a zero-termination in the result
string, leading to an incorrect result.
The routines in informix.c do not provide the length of their result
buffer, which would allow a replacement of strcpy() to safer strlcpy()
calls, but this requires an ABI breakage and that cannot happen in
back-branches.
Author: Oleg Tselebrovskiy
Reviewed-by: Ashutosh Bapat
Discussion: https://postgr.es/m/bf47888585149f83b276861a1662f7e4@postgrespro.ru
Backpatch-through: 12
We previously supposed that it was okay for different threads to
call bindtextdomain() concurrently (cf. commit 1f655fdc3).
It now emerges that there's at least one gettext implementation
in which that triggers an abort() crash, so let's stop doing that.
Add mutexes guarding libpq's and ecpglib's calls, which are the
only ones that need worry about multithreaded callers.
Note: in libpq, we could perhaps have piggybacked on
default_threadlock() to avoid defining a new mutex variable.
I judge that not terribly safe though, since libpq_gettext could
be called from code that is holding the default mutex. If that
were the first such call in the process, it'd fail. An extra
mutex is cheap insurance against unforeseen interactions.
Per bug #18312 from Christian Maurer. Back-patch to all
supported versions.
Discussion: https://postgr.es/m/18312-bbbabc8113592b78@postgresql.org
Discussion: https://postgr.es/m/264860.1707163416@sss.pgh.pa.us
Fix pthread-win32.h and pthread-win32.c to provide a more complete
emulation of POSIX pthread mutexes: define PTHREAD_MUTEX_INITIALIZER
and make sure that pthread_mutex_lock() can operate on a mutex
object that's been initialized that way. Then we don't need the
duplicative platform-specific logic in default_threadlock() and
pgtls_init(), which we'd otherwise need yet a third copy of for
an upcoming bug fix.
Also, since default_threadlock() supposes that pthread_mutex_lock()
cannot fail, try to ensure that that's actually true, by getting
rid of the malloc call that was formerly involved in initializing
an emulated mutex. We can define an extra state for the spinlock
field instead.
Also, replace the similar code in ecpglib/misc.c with this version.
While ecpglib's version at least had a POSIX-compliant API, it
also had the potential of failing during mutex init (but here,
because of CreateMutex failure rather than malloc failure). Since
all of misc.c's callers ignore failures, it seems like a wise idea
to avoid failures here too.
A further improvement in this area could be to unify libpq's and
ecpglib's implementations into a src/port/pthread-win32.c file.
But that doesn't seem like a bug fix, so I'll desist for now.
In preparation for the aforementioned bug fix, back-patch to all
supported branches.
Discussion: https://postgr.es/m/264860.1707163416@sss.pgh.pa.us
Commit b8ba7344e9 added in PQgetResult a derefence to a pointer
returned by pqPrepareAsyncResult(), before some other code that was
already testing that pointer for nullness. But since commit
618c16707a (in Postgres 15), pqPrepareAsyncResult() doesn't ever
return NULL (a statically-allocated result is returned if OOM). So in
branches 15 and up, we can remove the redundant pointer check with no
harm done.
However, in branch 14, pqPrepareAsyncResult() can indeed return NULL if
it runs out of memory. Fix things there by adding a null pointer check
before dereferencing the pointer. This should hint Coverity that the
preexisting check is not redundant but necessary.
Backpatch to 14, like b8ba7344e9.
Per Coverity.
OpenSSL will sometimes return SSL_ERROR_SYSCALL without having set
errno; this is apparently a reflection of recv(2)'s habit of not
setting errno when reporting EOF. Ensure that we treat such cases
the same as read EOF. Previously, we'd frequently report them like
"could not accept SSL connection: Success" which is confusing, or
worse report them with an unrelated errno left over from some
previous syscall.
To fix, ensure that errno is zeroed immediately before the call,
and report its value only when it's not zero afterwards; otherwise
report EOF.
For consistency, I've applied the same coding pattern in libpq's
pqsecure_raw_read(). Bare recv(2) shouldn't really return -1 without
setting errno, but in case it does we might as well cope.
Per report from Andres Freund. Back-patch to all supported versions.
Discussion: https://postgr.es/m/20231208181451.deqnflwxqoehhxpe@awork3.anarazel.de
The logic to keep the libpq command queue in sync with queries that have
been processed had a bug when errors were returned for reasons other
than problems in queries -- for example, when a connection is lost. We
incorrectly consumed an element from the command queue every time, but
this is wrong and can lead to the queue becoming empty ahead of time,
leading to later malfunction: PQgetResult would return nothing,
potentially causing the calling application to enter a busy loop.
Fix by making the SYNC queue element a barrier that can only be consumed
when a SYNC message is received.
Backpatch to 14.
Reported by: Иван Трофимов (Ivan Trofimov) <i.trofimow@yandex.ru>
Discussion: https://postgr.es/m/17948-fcace7557e449957@postgresql.org
We should have done it this way all along, but we accidentally got
away with using the wrong BIO field up until OpenSSL 3.2. There,
the library's BIO routines that we rely on use the "data" field
for their own purposes, and our conflicting use causes assorted
weird behaviors up to and including core dumps when SSL connections
are attempted. Switch to using the approved field for the purpose,
i.e. app_data.
While at it, remove our configure probes for BIO_get_data as well
as the fallback implementation. BIO_{get,set}_app_data have been
there since long before any OpenSSL version that we still support,
even in the back branches.
Also, update src/test/ssl/t/001_ssltests.pl to allow for a minor
change in an error message spelling that evidently came in with 3.2.
Tristan Partin and Bo Andreson. Back-patch to all supported branches.
Discussion: https://postgr.es/m/CAN55FZ1eDDYsYaL7mv+oSLUij2h_u6hvD4Qmv-7PK7jkji0uyQ@mail.gmail.com
The libpq code in charge of creating per-connection SSL objects was
prone to a race condition when loading the custom BIO methods needed by
my_SSL_set_fd(). As BIO methods are stored as a static variable, the
initialization of a connection could fail because it could be possible
to have one thread refer to my_bio_methods while it is being manipulated
by a second concurrent thread.
This error has been introduced by 8bb14cdd33, that has removed
ssl_config_mutex around the call of my_SSL_set_fd(), that itself sets
the custom BIO methods used in libpq. Like previously, the BIO method
initialization is now protected by the existing ssl_config_mutex, itself
initialized earlier for WIN32.
While on it, document that my_bio_methods is protected by
ssl_config_mutex, as this can be easy to miss.
Reported-by: Willi Mann
Author: Willi Mann, Michael Paquier
Discussion: https://postgr.es/m/e77abc4c-4d03-4058-a9d7-ef0035657e04@celonis.com
Backpatch-through: 12
When using GSSAPI encryption in non-blocking mode, libpq sometimes
failed with "GSSAPI caller failed to retransmit all data needing
to be retried". The cause is that pqPutMsgEnd rounds its transmit
request down to an even multiple of 8K, and sometimes that can lead
to not requesting a write of data that was requested to be written
(but reported as not written) earlier. That can upset pg_GSS_write's
logic for dealing with not-yet-written data, since it's possible
the data in question had already been incorporated into an encrypted
packet that we weren't able to send during the previous call.
We could fix this with a one-or-two-line hack to disable pqPutMsgEnd's
round-down behavior, but that seems like making the caller work around
a behavior that pg_GSS_write shouldn't expose in this way. Instead,
adjust pg_GSS_write to never report a partial write: it either
reports a complete write, or reflects the failure of the lower-level
pqsecure_raw_write call. The requirement still exists for the caller
to present at least as much data as on the previous call, but with
the caller-visible write start point not moving there is no temptation
for it to present less. We lose some ability to reclaim buffer space
early, but I doubt that that will make much difference in practice.
This also gets rid of a rather dubious assumption that "any
interesting failure condition (from pqsecure_raw_write) will recur
on the next try". We've not seen failure reports traceable to that,
but I've never trusted it particularly and am glad to remove it.
Make the same adjustments to the equivalent backend routine
be_gssapi_write(). It is probable that there's no bug on the backend
side, since we don't have a notion of nonblock mode there; but we
should keep the logic the same to ease future maintenance.
Per bug #18210 from Lars Kanis. Back-patch to all supported branches.
Discussion: https://postgr.es/m/18210-4c6d0b14627f2eb8@postgresql.org
When PQsendFlushRequest() was added by commit 69cf1d5429, we argued
against adding a PQflush() call in it[1]. This is still the right
decision: if the user wants a flush to occur, they can just call that.
However, we failed to realize that the message bytes could still be
given to the kernel for transmitting when this can be made without
blocking. That's what pqPipelineFlush() does, and it is done for every
single other message type sent by libpq, so do that.
(When the socket is in blocking mode this may indeed block, but that's
what all the other libpq message-sending routines do, too.)
[1] https://www.postgresql.org/message-id/202106252352.5ca4byasfun5%40alvherre.pgsql
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQTxZRevRWkKodE-SnJk1Yfm4eKT+8E4Cyq3MJ9YKTnNew@mail.gmail.com
Commit 6df7a9698b accidentally included two identical prototypes for
default_multirange_selectivi() and commit 086cf1458c added a break;
statement where one was already present, thus duplicating it. While
there is no bug caused by this, fix by removing the duplicated lines
as they provide no value.
Backpatch the fix for duplicate prototypes to v14 and the duplicate
break statement fix to all supported branches to avoid backpatching
hazards due to the removal.
Reported-by: Anton Voloshin <a.voloshin@postgrespro.ru>
Discussion: https://postgr.es/m/0e69cb60-0176-f6d0-7e15-6478b7d85724@postgrespro.ru
When compiled with -C ORACLE, ecpg_get_data() had a one-off issue where
it would incorrectly store the null terminator byte to str[-1] when
varcharsize is 0, which is something that can happen when using SQLDA.
This would eat 1 byte from the previous field stored, corrupting the
results generated.
All the callers of ecpg_get_data() estimate and allocate enough storage
for the data received, and the fix of this commit relies on this
assumption. Note that this maps to the case where no padding or
truncation is required.
This issue has been introduced by 3b7ab43 with the Oracle compatibility
option, so backpatch down to v11.
Author: Kyotaro Horiguchi
Discussion: https://postgr.es/m/20230410.173500.440060475837236886.horikyota.ntt@gmail.com
Backpatch-through: 11
The error cases for TLS and GSS encryption were inconsistent. After TLS
fails, the connection is marked as dead and follow-up calls of
PQconnectPoll() would return immediately, but GSS encryption was not
doing that, so the connection would still have been allowed to enter the
GSS handling code. This was handled incorrectly when gssencmode was set
to "require". "prefer" was working correctly, and this could not happen
under "disable" as GSS encryption would not be attempted.
This commit makes the error handling of GSS encryption on par with TLS
portion, fixing the case of gssencmode=require.
Reported-by: Jacob Champion
Author: Michael Paquier
Reviewed-by: Jacob Champion, Stephen Frost
Discussion: https://postgr.es/m/23787477-5fe1-a161-6d2a-e459f74c4713@timescale.com
Backpatch-through: 12
OpenSSL 1.1.1 and newer versions have added support for RSA-PSS
certificates, which requires the use of a specific routine in OpenSSL to
determine which hash function to use when compiling it when using
channel binding in SCRAM-SHA-256. X509_get_signature_nid(), that is the
original routine the channel binding code has relied on, is not able to
determine which hash algorithm to use for such certificates. However,
X509_get_signature_info(), new to OpenSSL 1.1.1, is able to do it. This
commit switches the channel binding logic to rely on
X509_get_signature_info() over X509_get_signature_nid(), which would be
the choice when building with 1.1.1 or newer.
The error could have been triggered on the client or the server, hence
libpq and the backend need to have their related code paths patched.
Note that attempting to load an RSA-PSS certificate with OpenSSL 1.1.0
or older leads to a failure due to an unsupported algorithm.
The discovery of relying on X509_get_signature_info() comes from Jacob,
the tests have been written by Heikki (with few tweaks from me), while I
have bundled the whole together while adding the bits needed for MSVC
and meson.
This issue exists since channel binding exists, so backpatch all the way
down. Some tests are added in 15~, triggered if compiling with OpenSSL
1.1.1 or newer, where the certificate and key files can easily be
generated for RSA-PSS.
Reported-by: Gunnar "Nick" Bluth
Author: Jacob Champion, Heikki Linnakangas
Discussion: https://postgr.es/m/17760-b6c61e752ec07060@postgresql.org
Backpatch-through: 11
pqsecure_open_gss() includes a code path handling error messages with
v2-style protocol messages coming from the server. The client-side
buffer holding the error message does not force a NULL-termination, with
the data of the server getting copied to the errorMessage of the
connection. Hence, it would be possible for a server to send an
unterminated string and copy arbitrary bytes in the buffer receiving the
error message in the client, opening the door to a crash or even data
exposure.
As at this stage of the authentication process the exchange has not been
completed yet, this could be abused by an attacker without Kerberos
credentials. Clients that have a valid kerberos cache are vulnerable as
libpq opportunistically requests for it except if gssencmode is
disabled.
Author: Jacob Champion
Backpatch-through: 12
Security: CVE-2022-41862
The frontend-side routine in charge of building a SCRAM verifier
mentioned that the restrictions applying to SASLprep on the password
with the encoding are described at the top of fe-auth-scram.c, but this
information is in auth-scram.c.
This is wrong since 8f8b9be, so backpatch all the way down as this is an
important documentation bit.
Spotted while reviewing a different patch.
Backpatch-through: 11
When a query whose results were requested in single-row mode is the last
in the queue by the time those results are being read, the single-row
flag was not being reset, because we were returning early from
pqPipelineProcessQueue. Move that stanza up so that the flag is always
reset at the end of sending that query's results.
Add a test for the situation.
Backpatch to 14.
Author: Denis Laxalde <denis.laxalde@dalibo.com>
Discussion: https://postgr.es/m/01af18c5-dacc-a8c8-07ee-aecc7650c3e8@dalibo.com
Commit ebc8b7d44 intended to change the behavior of
PQsslAttribute(NULL, "library"), but accidentally also changed
what happens with a non-NULL conn pointer. Undo that so that
only the intended behavior change happens. Clarify some
associated documentation.
Per bug #17625 from Heath Lord. Back-patch to v15.
Discussion: https://postgr.es/m/17625-fc47c78b7d71b534@postgresql.org
The extended query protocol implementation I added in commit
acb7e4eb6b has bugs when used in pipeline mode. Rather than spend
more time trying to fix it, remove that code and make the function rely
on simple query protocol only, meaning it can no longer be used in
pipeline mode.
Users can easily change their applications to use PQsendQueryParams
instead. We leave PQsendQuery in place for Postgres 14, just in case
somebody is using it and has not hit the mentioned bugs; but we should
recommend that it not be used.
Backpatch to 15.
Per bug report from Gabriele Varrazzo.
Discussion: https://postgr.es/m/CA+mi_8ZGSQNmW6-mk_iSR4JZB_LJ4ww3suOF+1vGNs3MrLsv4g@mail.gmail.com
The ECPG preprocessor converted code such as
static varchar str1[10], str2[20], str3[30];
into
static struct varchar_1 { int len; char arr[ 10 ]; } str1 ;
struct varchar_2 { int len; char arr[ 20 ]; } str2 ;
struct varchar_3 { int len; char arr[ 30 ]; } str3 ;
thus losing the storage attribute for the later variables.
Repeat the declaration for each such variable.
(Note that this occurred only for variables declared "varchar"
or "bytea", which may help explain how it escaped detection
for so long.)
Andrey Sokolov
Discussion: https://postgr.es/m/942241662288242@mail.yandex.ru
The reverts the following and makes some associated cleanups:
commit f79b803dc: Common SQL/JSON clauses
commit f4fb45d15: SQL/JSON constructors
commit 5f0adec25: Make STRING an unreserved_keyword.
commit 33a377608: IS JSON predicate
commit 1a36bc9db: SQL/JSON query functions
commit 606948b05: SQL JSON functions
commit 49082c2cc: RETURNING clause for JSON() and JSON_SCALAR()
commit 4e34747c8: JSON_TABLE
commit fadb48b00: PLAN clauses for JSON_TABLE
commit 2ef6f11b0: Reduce running time of jsonb_sqljson test
commit 14d3f24fa: Further improve jsonb_sqljson parallel test
commit a6baa4bad: Documentation for SQL/JSON features
commit b46bcf7a4: Improve readability of SQL/JSON documentation.
commit 112fdb352: Fix finalization for json_objectagg and friends
commit fcdb35c32: Fix transformJsonBehavior
commit 4cd8717af: Improve a couple of sql/json error messages
commit f7a605f63: Small cleanups in SQL/JSON code
commit 9c3d25e17: Fix JSON_OBJECTAGG uniquefying bug
commit a79153b7a: Claim SQL standard compliance for SQL/JSON features
commit a1e7616d6: Rework SQL/JSON documentation
commit 8d9f9634e: Fix errors in copyfuncs/equalfuncs support for JSON node types.
commit 3c633f32b: Only allow returning string types or bytea from json_serialize
commit 67b26703b: expression eval: Fix EEOP_JSON_CONSTRUCTOR and EEOP_JSONEXPR size.
The release notes are also adjusted.
Backpatch to release 15.
Discussion: https://postgr.es/m/40d2c882-bcac-19a9-754d-4299e1d87ac7@postgresql.org
There's a convention that externally-visible libpq functions should
check for a NULL PGconn pointer, and fail gracefully instead of
crashing. PQflush() and PQisnonblocking() didn't get that memo
though. Also add a similar check to PQdefaultSSLKeyPassHook_OpenSSL;
while it's not clear that ordinary usage could reach that with a
null conn pointer, it's cheap enough to check, so let's be consistent.
Daniele Varrazzo and Tom Lane
Discussion: https://postgr.es/m/CA+mi_8Zm_mVVyW1iNFgyMd9Oh0Nv8-F+7Y3-BqwMgTMHuo_h2Q@mail.gmail.com
The common recipe when TAP tests are disabled doesn't work, because the
libpq-specific recipe wants to define the PATH environment variable, so
the starting '@' is misinterpreted as part of the command instead of
silencing said command.
Fix by setting the environment variable in a way that doesn't interfere
with the recipe.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/20220720172321.GL12702@telsasoft.com
Commit 054325c5ee created a memory leak in PQsendQueryInternal in case
an error occurs while sending the message. Repair.
Backpatch to 14, like that commit. Reported by Coverity.
We were going into IDLE state too soon when executing queries via
PQsendQuery in pipeline mode, causing several scenarios to misbehave in
different ways -- most notably, as reported by Daniele Varrazzo, that a
warning message is produced by libpq:
message type 0x33 arrived from server while idle
But it is also possible, if queries are sent and results consumed not in
lockstep, for the expected mediating NULL result values from PQgetResult
to be lost (a problem which has not been reported, but which is more
serious).
Fix this by introducing two new concepts: one is a command queue element
PGQUERY_CLOSE to tell libpq to wait for the CloseComplete server
response to the Close message that is sent by PQsendQuery. Because the
application is not expecting any PGresult from this, the mechanism to
consume it is a bit hackish.
The other concept, authored by Horiguchi-san, is a PGASYNC_PIPELINE_IDLE
state for libpq's state machine to differentiate "really idle" from
merely "the idle state that occurs in between reading results from the
server for elements in the pipeline". This makes libpq not go fully
IDLE when the libpq command queue contains entries; in normal cases, we
only go IDLE once at the end of the pipeline, when the server response
to the final SYNC message is received. (However, there are corner cases
it doesn't fix, such as terminating the query sequence by
PQsendFlushRequest instead of PQpipelineSync; this sort of scenario is
what requires PGQUERY_CLOSE bit above.)
This last bit helps make the libpq state machine clearer; in particular
we can get rid of an ugly hack in pqParseInput3 to avoid considering
IDLE as such when the command queue contains entries.
A new test mode is added to libpq_pipeline.c to tickle some related
problematic cases.
Reported-by: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Co-authored-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/CA+mi_8bvD0_CW3sumgwPvWdNzXY32itoG_16tDYRu_1S2gV2iw@mail.gmail.com
Per buildfarm member prairiedog, this platform rejects uninitialized
global variables in shared libraries. Back-patch to v10, like the
addition of the variable.
Reviewed by Tom Lane.
Discussion: https://postgr.es/m/20220703030619.GB2378460@rfd.leadboat.com
ecpglib has been calling it once per SQL query and once per EXEC SQL GET
DESCRIPTOR. Instead, if newlocale() has not succeeded before, call it
while establishing a connection. This mitigates three problems:
- If newlocale() failed in EXEC SQL GET DESCRIPTOR, the command silently
proceeded without the intended locale change.
- On AIX, each newlocale()+freelocale() cycle leaked memory.
- newlocale() CPU usage may have been nontrivial.
Fail the connection attempt if newlocale() fails. Rearrange
ecpg_do_prologue() to validate the connection before its uselocale().
The sort of program that may regress is one running in an environment
where newlocale() fails. If that program establishes connections
without running SQL statements, it will stop working in response to this
change. I'm betting against the importance of such an ECPG use case.
Most SQL execution (any using ECPGdo()) has long required newlocale()
success, so there's little a connection could do without newlocale().
Back-patch to v10 (all supported versions).
Reviewed by Tom Lane. Reported by Guillaume Lelarge.
Discussion: https://postgr.es/m/20220101074055.GA54621@rfd.leadboat.com
Tom Lane