public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v25 8/9] Row pattern recognition patch (typedefs.list). 17+ messages / 6 participants [nested] [flat]
* [PATCH v25 8/9] Row pattern recognition patch (typedefs.list). @ 2024-12-21 06:19 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 17+ messages in thread From: Tatsuo Ishii @ 2024-12-21 06:19 UTC (permalink / raw) --- src/tools/pgindent/typedefs.list | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 971a150dbb..bf78e162bc 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -1690,6 +1690,7 @@ NamedLWLockTrancheRequest NamedTuplestoreScan NamedTuplestoreScanState NamespaceInfo +NavigationInfo NestLoop NestLoopParam NestLoopState @@ -2316,6 +2317,9 @@ RI_CompareKey RI_ConstraintInfo RI_QueryHashEntry RI_QueryKey +RPCommonSyntax +RPSkipTo +RPSubsetItem RTEKind RTEPermissionInfo RWConflict @@ -2670,6 +2674,7 @@ SimpleStringList SimpleStringListCell SingleBoundSortItem Size +SkipContext SkipPages SlabBlock SlabContext @@ -2761,6 +2766,7 @@ StreamStopReason String StringInfo StringInfoData +StringSet StripnullState SubLink SubLinkType @@ -3085,6 +3091,7 @@ VarString VarStringSortSupport Variable VariableAssignHook +VariablePos VariableSetKind VariableSetStmt VariableShowStmt -- 2.25.1 ----Next_Part(Sat_Dec_21_18_20_04_2024_526)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v25-0009-Allow-to-print-raw-parse-tree.patch" ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 16:02 Tom Lane <[email protected]> 0 siblings, 2 replies; 17+ messages in thread From: Tom Lane @ 2025-02-12 16:02 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > On 2025-02-11 13:32:32 -0300, Euler Taveira wrote: >> There is no bug. They are the same behind the scenes. > That *is* a bug. On windows the allocator that a shared library (i.e. libpq) > uses, may *not* be the same as the one that an executable > (i.e. pg_createsubscriber). It's not correct to free memory allocated in a > shared library just with free, it has to go through the library's free. Indeed. This is particularly pernicious because it will work even on Windows under common scenarios (which no doubt explains the lack of field reports). From the last time we discussed this [1]: It seems to work fine as long as a debug-readline is paired with a debug-psql or a release-readline is paired with a release-psql. I wish we had some way to detect misuses automatically ... This seems like the sort of bug that Coverity could detect if only it knew to look, but I have no idea if it could be configured that way. Maybe some weird lashup with a debugging malloc library would be another way. regards, tom lane [1] https://www.postgresql.org/message-id/[email protected] ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 16:46 Ranier Vilela <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 17+ messages in thread From: Ranier Vilela @ 2025-02-12 16:46 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; pgsql-hackers Hi. Em qua., 12 de fev. de 2025 às 13:02, Tom Lane <[email protected]> escreveu: > Andres Freund <[email protected]> writes: > > On 2025-02-11 13:32:32 -0300, Euler Taveira wrote: > >> There is no bug. They are the same behind the scenes. > > > That *is* a bug. On windows the allocator that a shared library (i.e. > libpq) > > uses, may *not* be the same as the one that an executable > > (i.e. pg_createsubscriber). It's not correct to free memory allocated > in a > > shared library just with free, it has to go through the library's free. > > Indeed. This is particularly pernicious because it will work even on > Windows under common scenarios (which no doubt explains the lack of > field reports). From the last time we discussed this [1]: > > It seems to work fine as long as a debug-readline is paired with a > debug-psql or a release-readline is paired with a release-psql. > > I wish we had some way to detect misuses automatically ... > Unfortunately, for now, there is only manual search. In fact, after your post, I did a new search and found two other occurrences. (src/bin/pg_amcheck/pg_amcheck.c) One fixed in the patch attached. Another *dat->amcheck_schema* is it not worth the effort, since the memory is not released at any point? Trivial patch attached. best regards, Ranier Vilela Attachments: [application/octet-stream] fix-api-misuse-pg_amcheck.patch (371B, ../../CAEudQArD_nKSnYCNUZiPPsJ2tNXgRmLbXGSOrH1vpOF_XtP0Vg@mail.gmail.com/3-fix-api-misuse-pg_amcheck.patch) download | inline diff: diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index c5ec25be22..dda8b1b8ad 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -560,7 +560,7 @@ main(int argc, char *argv[]) executeCommand(conn, install_sql, opts.echo); pfree(install_sql); - pfree(schema); + PQfreemem(schema); } /* ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 17:00 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 2 replies; 17+ messages in thread From: Andres Freund @ 2025-02-12 17:00 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Hi, On 2025-02-12 11:02:04 -0500, Tom Lane wrote: > I wish we had some way to detect misuses automatically ... > > This seems like the sort of bug that Coverity could detect if only it > knew to look, but I have no idea if it could be configured that way. > Maybe some weird lashup with a debugging malloc library would be > another way. Recent gcc actually has a fairly good way to detect this kind of issue: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute in particular, the variant of the attribute with "deallocator". I'd started on a patch to add those, but ran out of cycles for 18. I quickly checked out my branch and added the relevant attributes to PQescapeLiteral(), PQescapeIdentifier() and that indeed finds the issue pointed out in this thread: ../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c: In function 'create_logical_replication_slot': ../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c:1332:9: warning: 'pg_free' called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc] 1332 | pg_free(slot_name_esc); | ^~~~~~~~~~~~~~~~~~~~~~ ../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_createsubscriber.c:1326:25: note: returned from 'PQescapeLiteral' 1326 | slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ... but also finds one more: [62/214 42 28%] Compiling C object src/bin/pg_amcheck/pg_amcheck.p/pg_amcheck.c.o ../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c: In function 'main': ../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c:563:25: warning: 'pfree' called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc] 563 | pfree(schema); | ^~~~~~~~~~~~~ ../../../../../home/andres/src/postgresql/src/bin/pg_amcheck/pg_amcheck.c:556:34: note: returned from 'PQescapeIdentifier' 556 | schema = PQescapeIdentifier(conn, opts.install_schema, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 557 | strlen(opts.install_schema)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Note that that doesn't just require adding the attributes to PQescapeIdentifier() etc, but also to pg_malloc(), as the latter is how gcc knows that pg_pfree() is a deallocating function. Particularly for something like libpq it's not quitetrivial to add attributes like this, of course. We can't even depend on pg_config.h. One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's "armed" by a commandline -D flag, if the compiler is supported? Greetings, Andres Freund ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 17:17 Dagfinn Ilmari Mannsåker <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 17+ messages in thread From: Dagfinn Ilmari Mannsåker @ 2025-02-12 17:17 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > Hi, > > On 2025-02-12 11:02:04 -0500, Tom Lane wrote: >> I wish we had some way to detect misuses automatically ... >> >> This seems like the sort of bug that Coverity could detect if only it >> knew to look, but I have no idea if it could be configured that way. >> Maybe some weird lashup with a debugging malloc library would be >> another way. > > Recent gcc actually has a fairly good way to detect this kind of issue: > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute > in particular, the variant of the attribute with "deallocator". [...] > Note that that doesn't just require adding the attributes to > PQescapeIdentifier() etc, but also to pg_malloc(), as the latter is how gcc > knows that pg_pfree() is a deallocating function. > > > Particularly for something like libpq it's not quitetrivial to add > attributes like this, of course. We can't even depend on pg_config.h. > > One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's > "armed" by a commandline -D flag, if the compiler is supported? Does it need a -D flag, wouldn't __has_attribute(malloc) (with the fallback definition in c.h) be enough? - ilmari ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 17:23 Tom Lane <[email protected]> parent: Dagfinn Ilmari Mannsåker <[email protected]> 0 siblings, 1 reply; 17+ messages in thread From: Tom Lane @ 2025-02-12 17:23 UTC (permalink / raw) To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <[email protected]> writes: > Andres Freund <[email protected]> writes: >> Particularly for something like libpq it's not quitetrivial to add >> attributes like this, of course. We can't even depend on pg_config.h. >> One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's >> "armed" by a commandline -D flag, if the compiler is supported? > Does it need a -D flag, wouldn't __has_attribute(malloc) (with the > fallback definition in c.h) be enough? libpq-fe.h has to be compilable by application code that has never heard of pg_config.h let alone c.h, so we'd have to tread carefully about not breaking that property. But it seems like this would be worth looking into. regards, tom lane ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 17:29 Dagfinn Ilmari Mannsåker <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 17+ messages in thread From: Dagfinn Ilmari Mannsåker @ 2025-02-12 17:29 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Tom Lane <[email protected]> writes: > =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <[email protected]> writes: >> Andres Freund <[email protected]> writes: >>> Particularly for something like libpq it's not quitetrivial to add >>> attributes like this, of course. We can't even depend on pg_config.h. >>> One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's >>> "armed" by a commandline -D flag, if the compiler is supported? > >> Does it need a -D flag, wouldn't __has_attribute(malloc) (with the >> fallback definition in c.h) be enough? > > libpq-fe.h has to be compilable by application code that has never > heard of pg_config.h let alone c.h, so we'd have to tread carefully > about not breaking that property. But it seems like this would be > worth looking into. The fallback code isn't exactly complicated, so we could just duplicate it in libpq-fe.h: #ifndef __has_attribute #define __has_attribute(attribute) 0 #endif And then do something like this: #if __has_attribute (malloc) #define pg_attribute_malloc __attribute__((malloc)) #define pg_attribute_deallocator(...) __attribute__((malloc(__VA_ARGS__))) #else #define pg_attribute_malloc #define pg_attribute_deallocator(...) #endif > regards, tom lane - ilmari ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 17:38 Tom Lane <[email protected]> parent: Dagfinn Ilmari Mannsåker <[email protected]> 0 siblings, 2 replies; 17+ messages in thread From: Tom Lane @ 2025-02-12 17:38 UTC (permalink / raw) To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <[email protected]> writes: > Tom Lane <[email protected]> writes: >> libpq-fe.h has to be compilable by application code that has never >> heard of pg_config.h let alone c.h, so we'd have to tread carefully >> about not breaking that property. But it seems like this would be >> worth looking into. > The fallback code isn't exactly complicated, so we could just duplicate > it in libpq-fe.h: > #ifndef __has_attribute > #define __has_attribute(attribute) 0 > #endif The problem with that approach is the likelihood of stomping on symbols that a calling application will use later. I think we really need a controlling #ifdef check on some PG_FOO symbol that we can be sure no outside application will have defined. Roughly speaking, #ifdef PG_USE_ALLOCATOR_CHECKS #define pg_attribute_malloc __attribute__((malloc)) ... #else #define pg_attribute_malloc ... #endif and then we could make definition of PG_USE_ALLOCATOR_CHECKS be conditional on having the right compiler behavior, rather than trusting that a nest of #ifdef checks is sufficient to detect that. regards, tom lane ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 18:35 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 17+ messages in thread From: Tom Lane @ 2025-02-12 18:35 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers I experimented with the other approach: hack libpq.so to depend on dmalloc, leaving the rest of the system alone, so that libpq's allocations could not be freed elsewhere nor vice versa. It could not even get through initdb, crashing here: replace_guc_value(char **lines, const char *guc_name, const char *guc_value, bool mark_as_comment) { PQExpBuffer newline = createPQExpBuffer(); ... free(newline); /* but don't free newline->data */ which upon investigation is expected, because createPQExpBuffer is exported by libpq and therefore is returning space allocated within the shlib. Diking out that particular free() call just allows it to crash a bit later on, because initdb is totally full of direct manipulations of PQExpBuffer contents. This indicates to me that we have a *far* larger problem than we thought, if we'd like to be totally clean on this point. Realistically, it's not going to happen that way; it's just not worth the trouble and notational mess. I think if we're honest we should just document that such-and-such combinations of libpq and our client programs will work on Windows. For amusement's sake, totally dirty hack-and-slash patch attached. (I tested this on macOS, with dmalloc from MacPorts; adjust SHLIB_LINK to suit on other platforms.) regards, tom lane Attachments: [text/x-diff] use-dmalloc-within-libpq-hack-hack-hack.patch (2.5K, ../../[email protected]/2-use-dmalloc-within-libpq-hack-hack-hack.patch) download | inline diff: diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile index 701810a272a..91dc2e14d99 100644 --- a/src/interfaces/libpq/Makefile +++ b/src/interfaces/libpq/Makefile @@ -81,7 +81,7 @@ endif # that are built correctly for use in a shlib. SHLIB_LINK_INTERNAL = -lpgcommon_shlib -lpgport_shlib ifneq ($(PORTNAME), win32) -SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS) +SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) -L/opt/local/lib -ldmalloc $(PTHREAD_LIBS) else SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi32 -lssl -lsocket -lnsl -lresolv -lintl -lm $(PTHREAD_LIBS), $(LIBS)) $(LDAP_LIBS_FE) endif @@ -116,11 +116,6 @@ backend_src = $(top_srcdir)/src/backend # coding rule. libpq-refs-stamp: $(shlib) ifneq ($(enable_coverage), yes) -ifeq (,$(filter solaris,$(PORTNAME))) - @if nm -A -u $< 2>/dev/null | grep -v -e __cxa_atexit -e __tsan_func_exit | grep exit; then \ - echo 'libpq must not be calling any function which invokes exit'; exit 1; \ - fi -endif endif touch $@ diff --git a/src/interfaces/libpq/fe-auth-sasl.h b/src/interfaces/libpq/fe-auth-sasl.h index f06f547c07d..1abafe8d34b 100644 --- a/src/interfaces/libpq/fe-auth-sasl.h +++ b/src/interfaces/libpq/fe-auth-sasl.h @@ -146,7 +146,7 @@ typedef struct pg_fe_sasl_mech * state: The opaque mechanism state returned by init() *-------- */ - void (*free) (void *state); + void (*saslfree) (void *state); } pg_fe_sasl_mech; diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 85d1ca2864f..56ff7bc3b9d 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -573,7 +573,7 @@ pqDropConnection(PGconn *conn, bool flushInput) #endif if (conn->sasl_state) { - conn->sasl->free(conn->sasl_state); + conn->sasl->saslfree(conn->sasl_state); conn->sasl_state = NULL; } } diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 2546f9f8a50..7296d00b34f 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -20,6 +20,8 @@ #ifndef LIBPQ_INT_H #define LIBPQ_INT_H +#include <dmalloc.h> + /* We assume libpq-fe.h has already been included. */ #include "libpq-events.h" ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 18:49 Dagfinn Ilmari Mannsåker <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 17+ messages in thread From: Dagfinn Ilmari Mannsåker @ 2025-02-12 18:49 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Tom Lane <[email protected]> writes: > =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <[email protected]> writes: >> Tom Lane <[email protected]> writes: >>> libpq-fe.h has to be compilable by application code that has never >>> heard of pg_config.h let alone c.h, so we'd have to tread carefully >>> about not breaking that property. But it seems like this would be >>> worth looking into. > >> The fallback code isn't exactly complicated, so we could just duplicate >> it in libpq-fe.h: > >> #ifndef __has_attribute >> #define __has_attribute(attribute) 0 >> #endif > > The problem with that approach is the likelihood of stomping on > symbols that a calling application will use later. I think we > really need a controlling #ifdef check on some PG_FOO symbol > that we can be sure no outside application will have defined. > Roughly speaking, > > #ifdef PG_USE_ALLOCATOR_CHECKS As long as we're not checking for too many attributes, we could avoid introducing the __has_attribute symbol by doing: #if defined(__has_attribute) && __has_attribute(malloc) > #define pg_attribute_malloc __attribute__((malloc)) > ... > #else > #define pg_attribute_malloc > ... > #endif > > and then we could make definition of PG_USE_ALLOCATOR_CHECKS > be conditional on having the right compiler behavior, rather > than trusting that a nest of #ifdef checks is sufficient to > detect that. But I just looked at the clang attribute docs (https://clang.llvm.org/docs/AttributeReference.html#malloc), and it only has the argumentless version, not the one that that declares the matching deallocator, so you're right we need a more comprehensive check. > regards, tom lane - ilmari ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 23:56 Michael Paquier <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 17+ messages in thread From: Michael Paquier @ 2025-02-12 23:56 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers On Wed, Feb 12, 2025 at 12:00:03PM -0500, Andres Freund wrote: > Particularly for something like libpq it's not quitetrivial to add > attributes like this, of course. We can't even depend on pg_config.h. > > One way would be to define them in libpq-fe.h, guarded by an #ifdef, that's > "armed" by a commandline -D flag, if the compiler is supported? Interesting set of tricks. I have looked at bit at the uses of PQescapeLiteral() and PQescapeIdentifier() in the tree. On top of the one in pg_amcheck you are just pointing to, there is an inconsistency in pg_upgrade.c for set_locale_and_encoding() where datlocale_literal may be allocated with a pg_strdup() or a PQescapeLiteral() depending on the path. The code has been using PQfreemem() for the pg_strdup() allocation, which is logically incorrect. The pg_upgrade path is fancy as designed this way, for sure, but that's less invasive than hardcoding three times "NULL". Any thoughts about backpatching something like that for both of them? -- Michael Attachments: [text/x-diff] libpq-escape-fixes.patch (847B, ../../[email protected]/2-libpq-escape-fixes.patch) download | inline diff: diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index c5ec25be22b..dda8b1b8ade 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -560,7 +560,7 @@ main(int argc, char *argv[]) executeCommand(conn, install_sql, opts.echo); pfree(install_sql); - pfree(schema); + PQfreemem(schema); } /* diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index 36c7f3879d5..d854ad110ee 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -465,7 +465,10 @@ set_locale_and_encoding(void) PQfreemem(datcollate_literal); PQfreemem(datctype_literal); - PQfreemem(datlocale_literal); + if (locale->db_locale) + PQfreemem(datlocale_literal); + else + pg_free(datlocale_literal); PQfinish(conn_new_template1); [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-12 23:59 Michael Paquier <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 17+ messages in thread From: Michael Paquier @ 2025-02-12 23:59 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers On Wed, Feb 12, 2025 at 01:35:20PM -0500, Tom Lane wrote: > For amusement's sake, totally dirty hack-and-slash patch attached. > (I tested this on macOS, with dmalloc from MacPorts; adjust SHLIB_LINK > to suit on other platforms.) Ugh. Fun one. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-13 00:08 Tom Lane <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 2 replies; 17+ messages in thread From: Tom Lane @ 2025-02-13 00:08 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Michael Paquier <[email protected]> writes: > I have looked at bit at the uses of PQescapeLiteral() and > PQescapeIdentifier() in the tree. On top of the one in pg_amcheck you > are just pointing to, there is an inconsistency in pg_upgrade.c for > set_locale_and_encoding() where datlocale_literal may be allocated > with a pg_strdup() or a PQescapeLiteral() depending on the path. The > code has been using PQfreemem() for the pg_strdup() allocation, which > is logically incorrect. Yeah, I suspected there would be places like that. It just hasn't mattered in practice up to now. (I have a vague recollection that Windows used to be pickier about this, but evidently not in recent years.) I spent a little time earlier today seeing what I could do with the use-dmalloc patch I posted earlier. It turns out you can get through initdb after s/free/PQfreemem/ in just two places, and then the backend works fine. But psql is a frickin' disaster --- there's free's of strings made with PQExpBuffer all over its backslash-command handling, and no easy way to clean it up. Maybe other clients will be less of a mess, but I'm not betting on that. regards, tom lane ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-13 04:50 Michael Paquier <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 17+ messages in thread From: Michael Paquier @ 2025-02-13 04:50 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers On Wed, Feb 12, 2025 at 07:08:31PM -0500, Tom Lane wrote: > I spent a little time earlier today seeing what I could do with the > use-dmalloc patch I posted earlier. It turns out you can get through > initdb after s/free/PQfreemem/ in just two places, and then the > backend works fine. But psql is a frickin' disaster --- there's > free's of strings made with PQExpBuffer all over its backslash-command > handling, and no easy way to clean it up. Maybe other clients will > be less of a mess, but I'm not betting on that. Hmm. Okay. It sounds like it would be better to group everything that has been pointed together towards what should be a more generic solution than what I have posted. So I am holding on touching anything. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-25 06:02 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 17+ messages in thread From: Michael Paquier @ 2025-02-25 06:02 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers On Thu, Feb 13, 2025 at 01:50:29PM +0900, Michael Paquier wrote: > On Wed, Feb 12, 2025 at 07:08:31PM -0500, Tom Lane wrote: >> I spent a little time earlier today seeing what I could do with the >> use-dmalloc patch I posted earlier. It turns out you can get through >> initdb after s/free/PQfreemem/ in just two places, and then the >> backend works fine. But psql is a frickin' disaster --- there's >> free's of strings made with PQExpBuffer all over its backslash-command >> handling, and no easy way to clean it up. Maybe other clients will >> be less of a mess, but I'm not betting on that. > > Hmm. Okay. It sounds like it would be better to group everything > that has been pointed together towards what should be a more generic > solution than what I have posted. So I am holding on touching > anything. Two weeks later. Would there be any objections for doing something in the lines of [1] for pg_upgrade and pg_amcheck? The pg_upgrade bit is a bit strange, for sure, still it's better than the current state of things. [1]: https://www.postgresql.org/message-id/[email protected] -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-25 11:54 Ranier Vilela <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 17+ messages in thread From: Ranier Vilela @ 2025-02-25 11:54 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Euler Taveira <[email protected]>; pgsql-hackers Em ter., 25 de fev. de 2025 às 03:02, Michael Paquier <[email protected]> escreveu: > On Thu, Feb 13, 2025 at 01:50:29PM +0900, Michael Paquier wrote: > > On Wed, Feb 12, 2025 at 07:08:31PM -0500, Tom Lane wrote: > >> I spent a little time earlier today seeing what I could do with the > >> use-dmalloc patch I posted earlier. It turns out you can get through > >> initdb after s/free/PQfreemem/ in just two places, and then the > >> backend works fine. But psql is a frickin' disaster --- there's > >> free's of strings made with PQExpBuffer all over its backslash-command > >> handling, and no easy way to clean it up. Maybe other clients will > >> be less of a mess, but I'm not betting on that. > > > > Hmm. Okay. It sounds like it would be better to group everything > > that has been pointed together towards what should be a more generic > > solution than what I have posted. So I am holding on touching > > anything. > > Two weeks later. Would there be any objections for doing something in > the lines of [1] for pg_upgrade and pg_amcheck? I prefer not to mix api. We already have a branch for decision. Let's use it then. diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index d95c491fb5..b2c8e8e420 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -455,7 +455,9 @@ set_locale_and_encoding(void) locale->db_locale, strlen(locale->db_locale)); else - datlocale_literal = pg_strdup("NULL"); + datlocale_literal = PQescapeLiteral(conn_new_template1, + "NULL", + strlen("NULL")); best regards, Ranier Vilela Attachments: [application/octet-stream] avoid-mix-api-pg_upgrade.patch (571B, ../../CAEudQAqsjfhfJYp8CWTJk3u-7h4C2LL38StJgcrFvcnc6+cOCA@mail.gmail.com/3-avoid-mix-api-pg_upgrade.patch) download | inline diff: diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index d95c491fb5..b2c8e8e420 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -455,7 +455,9 @@ set_locale_and_encoding(void) locale->db_locale, strlen(locale->db_locale)); else - datlocale_literal = pg_strdup("NULL"); + datlocale_literal = PQescapeLiteral(conn_new_template1, + "NULL", + strlen("NULL")); /* update template0 in new cluster */ if (GET_MAJOR_VERSION(new_cluster.major_version) >= 1700) ^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Small memory fixes for pg_createsubcriber @ 2025-02-25 12:39 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 17+ messages in thread From: Andres Freund @ 2025-02-25 12:39 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; Euler Taveira <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Hi, On 2025-02-12 19:08:31 -0500, Tom Lane wrote: > Michael Paquier <[email protected]> writes: > > I have looked at bit at the uses of PQescapeLiteral() and > > PQescapeIdentifier() in the tree. On top of the one in pg_amcheck you > > are just pointing to, there is an inconsistency in pg_upgrade.c for > > set_locale_and_encoding() where datlocale_literal may be allocated > > with a pg_strdup() or a PQescapeLiteral() depending on the path. The > > code has been using PQfreemem() for the pg_strdup() allocation, which > > is logically incorrect. > > Yeah, I suspected there would be places like that. It just hasn't > mattered in practice up to now. (I have a vague recollection that > Windows used to be pickier about this, but evidently not in recent > years.) It's a question of compiler / link options. If you e.g. have a debug psql runtime linked against a non-debug libpq, the allocators for both will differ. Or if you link an application statically while a library is built for a shared C runtime. Or a few other such things. But when building in the BF / CI we'll not typically encounter this issue between libraries and binaries in our source tree, because they'll all be built the same. However, we have more recently hit this with external libraries we link to. While it's not recommended, we commonly build (in BF/CI) a debug postgres against production openssl, zstd etc. Which means they don't share allocators, file descriptors etc. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2025-02-25 12:39 UTC | newest] Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-12-21 06:19 [PATCH v25 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]> 2025-02-12 16:02 Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]> 2025-02-12 16:46 ` Re: Small memory fixes for pg_createsubcriber Ranier Vilela <[email protected]> 2025-02-12 17:00 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[email protected]> 2025-02-12 17:17 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]> 2025-02-12 17:23 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]> 2025-02-12 17:29 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]> 2025-02-12 17:38 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]> 2025-02-12 18:35 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]> 2025-02-12 23:59 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]> 2025-02-12 18:49 ` Re: Small memory fixes for pg_createsubcriber Dagfinn Ilmari Mannsåker <[email protected]> 2025-02-12 23:56 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]> 2025-02-13 00:08 ` Re: Small memory fixes for pg_createsubcriber Tom Lane <[email protected]> 2025-02-13 04:50 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]> 2025-02-25 06:02 ` Re: Small memory fixes for pg_createsubcriber Michael Paquier <[email protected]> 2025-02-25 11:54 ` Re: Small memory fixes for pg_createsubcriber Ranier Vilela <[email protected]> 2025-02-25 12:39 ` Re: Small memory fixes for pg_createsubcriber Andres Freund <[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