public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v16 15/16] tests: Rename conflicting role names 5+ messages / 3 participants [nested] [flat]
* [PATCH v16 15/16] tests: Rename conflicting role names @ 2022-09-25 23:49 Andres Freund <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Andres Freund @ 2022-09-25 23:49 UTC (permalink / raw) These cause problems when running installcheck concurrently. --- src/pl/plperl/expected/plperl_setup.out | 30 +++++++++---------- src/pl/plperl/sql/plperl_setup.sql | 30 +++++++++---------- contrib/adminpack/expected/adminpack.out | 20 ++++++------- contrib/adminpack/sql/adminpack.sql | 20 ++++++------- .../passwordcheck/expected/passwordcheck.out | 16 +++++----- contrib/passwordcheck/sql/passwordcheck.sql | 16 +++++----- 6 files changed, 66 insertions(+), 66 deletions(-) diff --git a/src/pl/plperl/expected/plperl_setup.out b/src/pl/plperl/expected/plperl_setup.out index 5234febefd6..5537a95ccc2 100644 --- a/src/pl/plperl/expected/plperl_setup.out +++ b/src/pl/plperl/expected/plperl_setup.out @@ -4,9 +4,9 @@ -- Before going ahead with the to-be-tested installations, verify that -- a non-superuser is allowed to install plperl (but not plperlu) when -- suitable permissions have been granted. -CREATE USER regress_user1; -CREATE USER regress_user2; -SET ROLE regress_user1; +CREATE USER regress_plperl_user1; +CREATE USER regress_plperl_user2; +SET ROLE regress_plperl_user1; CREATE EXTENSION plperl; -- fail ERROR: permission denied to create extension "plperl" HINT: Must have CREATE privilege on current database to create this extension. @@ -16,18 +16,18 @@ HINT: Must be superuser to create this extension. RESET ROLE; DO $$ begin - execute format('grant create on database %I to regress_user1', + execute format('grant create on database %I to regress_plperl_user1', current_database()); end; $$; -SET ROLE regress_user1; +SET ROLE regress_plperl_user1; CREATE EXTENSION plperl; CREATE EXTENSION plperlu; -- fail ERROR: permission denied to create extension "plperlu" HINT: Must be superuser to create this extension. CREATE SCHEMA plperl_setup_scratch; SET search_path = plperl_setup_scratch; -GRANT ALL ON SCHEMA plperl_setup_scratch TO regress_user2; +GRANT ALL ON SCHEMA plperl_setup_scratch TO regress_plperl_user2; CREATE FUNCTION foo1() returns int language plperl as '1;'; SELECT foo1(); foo1 @@ -38,15 +38,15 @@ SELECT foo1(); -- Must reconnect to avoid failure with non-MULTIPLICITY Perl interpreters \c - SET search_path = plperl_setup_scratch; -SET ROLE regress_user1; +SET ROLE regress_plperl_user1; -- Should be able to change privileges on the language revoke all on language plperl from public; -SET ROLE regress_user2; +SET ROLE regress_plperl_user2; CREATE FUNCTION foo2() returns int language plperl as '2;'; -- fail ERROR: permission denied for language plperl -SET ROLE regress_user1; -grant usage on language plperl to regress_user2; -SET ROLE regress_user2; +SET ROLE regress_plperl_user1; +grant usage on language plperl to regress_plperl_user2; +SET ROLE regress_plperl_user2; CREATE FUNCTION foo2() returns int language plperl as '2;'; SELECT foo2(); foo2 @@ -54,7 +54,7 @@ SELECT foo2(); 2 (1 row) -SET ROLE regress_user1; +SET ROLE regress_plperl_user1; -- Should be able to drop the extension, but not the language per se DROP LANGUAGE plperl CASCADE; ERROR: cannot drop language plperl because extension plperl requires it @@ -65,9 +65,9 @@ DETAIL: drop cascades to function foo1() drop cascades to function foo2() -- Clean up RESET ROLE; -DROP OWNED BY regress_user1; -DROP USER regress_user1; -DROP USER regress_user2; +DROP OWNED BY regress_plperl_user1; +DROP USER regress_plperl_user1; +DROP USER regress_plperl_user2; -- Now install the versions that will be used by subsequent test scripts. CREATE EXTENSION plperl; CREATE EXTENSION plperlu; diff --git a/src/pl/plperl/sql/plperl_setup.sql b/src/pl/plperl/sql/plperl_setup.sql index a89cf56617e..0eac9156294 100644 --- a/src/pl/plperl/sql/plperl_setup.sql +++ b/src/pl/plperl/sql/plperl_setup.sql @@ -6,10 +6,10 @@ -- a non-superuser is allowed to install plperl (but not plperlu) when -- suitable permissions have been granted. -CREATE USER regress_user1; -CREATE USER regress_user2; +CREATE USER regress_plperl_user1; +CREATE USER regress_plperl_user2; -SET ROLE regress_user1; +SET ROLE regress_plperl_user1; CREATE EXTENSION plperl; -- fail CREATE EXTENSION plperlu; -- fail @@ -18,18 +18,18 @@ RESET ROLE; DO $$ begin - execute format('grant create on database %I to regress_user1', + execute format('grant create on database %I to regress_plperl_user1', current_database()); end; $$; -SET ROLE regress_user1; +SET ROLE regress_plperl_user1; CREATE EXTENSION plperl; CREATE EXTENSION plperlu; -- fail CREATE SCHEMA plperl_setup_scratch; SET search_path = plperl_setup_scratch; -GRANT ALL ON SCHEMA plperl_setup_scratch TO regress_user2; +GRANT ALL ON SCHEMA plperl_setup_scratch TO regress_plperl_user2; CREATE FUNCTION foo1() returns int language plperl as '1;'; SELECT foo1(); @@ -38,25 +38,25 @@ SELECT foo1(); \c - SET search_path = plperl_setup_scratch; -SET ROLE regress_user1; +SET ROLE regress_plperl_user1; -- Should be able to change privileges on the language revoke all on language plperl from public; -SET ROLE regress_user2; +SET ROLE regress_plperl_user2; CREATE FUNCTION foo2() returns int language plperl as '2;'; -- fail -SET ROLE regress_user1; +SET ROLE regress_plperl_user1; -grant usage on language plperl to regress_user2; +grant usage on language plperl to regress_plperl_user2; -SET ROLE regress_user2; +SET ROLE regress_plperl_user2; CREATE FUNCTION foo2() returns int language plperl as '2;'; SELECT foo2(); -SET ROLE regress_user1; +SET ROLE regress_plperl_user1; -- Should be able to drop the extension, but not the language per se DROP LANGUAGE plperl CASCADE; @@ -64,9 +64,9 @@ DROP EXTENSION plperl CASCADE; -- Clean up RESET ROLE; -DROP OWNED BY regress_user1; -DROP USER regress_user1; -DROP USER regress_user2; +DROP OWNED BY regress_plperl_user1; +DROP USER regress_plperl_user1; +DROP USER regress_plperl_user2; -- Now install the versions that will be used by subsequent test scripts. CREATE EXTENSION plperl; diff --git a/contrib/adminpack/expected/adminpack.out b/contrib/adminpack/expected/adminpack.out index 76aafe6316a..6bcf2217f25 100644 --- a/contrib/adminpack/expected/adminpack.out +++ b/contrib/adminpack/expected/adminpack.out @@ -36,10 +36,10 @@ SELECT pg_read_file('test_file1'); -- disallowed file paths for non-superusers and users who are -- not members of pg_write_server_files -CREATE ROLE regress_user1; -GRANT pg_read_all_settings TO regress_user1; -GRANT EXECUTE ON FUNCTION pg_file_write(text,text,bool) TO regress_user1; -SET ROLE regress_user1; +CREATE ROLE regress_adminpack_user1; +GRANT pg_read_all_settings TO regress_adminpack_user1; +GRANT EXECUTE ON FUNCTION pg_file_write(text,text,bool) TO regress_adminpack_user1; +SET ROLE regress_adminpack_user1; SELECT pg_file_write('../test_file0', 'test0', false); ERROR: path must be in or below the current directory SELECT pg_file_write('/tmp/test_file0', 'test0', false); @@ -53,9 +53,9 @@ SELECT pg_file_write(current_setting('data_directory') || '/test_file4', 'test4' SELECT pg_file_write(current_setting('data_directory') || '/../test_file4', 'test4', false); ERROR: absolute path not allowed RESET ROLE; -REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,bool) FROM regress_user1; -REVOKE pg_read_all_settings FROM regress_user1; -DROP ROLE regress_user1; +REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,bool) FROM regress_adminpack_user1; +REVOKE pg_read_all_settings FROM regress_adminpack_user1; +DROP ROLE regress_adminpack_user1; -- sync SELECT pg_file_sync('test_file1'); -- sync file pg_file_sync @@ -153,8 +153,8 @@ SELECT pg_file_unlink('test_file4'); (1 row) -- superuser checks -CREATE USER regress_user1; -SET ROLE regress_user1; +CREATE USER regress_adminpack_user1; +SET ROLE regress_adminpack_user1; SELECT pg_file_write('test_file0', 'test0', false); ERROR: permission denied for function pg_file_write SELECT pg_file_sync('test_file0'); @@ -167,6 +167,6 @@ ERROR: permission denied for function pg_file_unlink SELECT pg_logdir_ls(); ERROR: permission denied for function pg_logdir_ls RESET ROLE; -DROP USER regress_user1; +DROP USER regress_adminpack_user1; -- no further tests for pg_logdir_ls() because it depends on the -- server's logging setup diff --git a/contrib/adminpack/sql/adminpack.sql b/contrib/adminpack/sql/adminpack.sql index 918d0bdc65e..5776c9af0d1 100644 --- a/contrib/adminpack/sql/adminpack.sql +++ b/contrib/adminpack/sql/adminpack.sql @@ -14,20 +14,20 @@ SELECT pg_read_file('test_file1'); -- disallowed file paths for non-superusers and users who are -- not members of pg_write_server_files -CREATE ROLE regress_user1; +CREATE ROLE regress_adminpack_user1; -GRANT pg_read_all_settings TO regress_user1; -GRANT EXECUTE ON FUNCTION pg_file_write(text,text,bool) TO regress_user1; +GRANT pg_read_all_settings TO regress_adminpack_user1; +GRANT EXECUTE ON FUNCTION pg_file_write(text,text,bool) TO regress_adminpack_user1; -SET ROLE regress_user1; +SET ROLE regress_adminpack_user1; SELECT pg_file_write('../test_file0', 'test0', false); SELECT pg_file_write('/tmp/test_file0', 'test0', false); SELECT pg_file_write(current_setting('data_directory') || '/test_file4', 'test4', false); SELECT pg_file_write(current_setting('data_directory') || '/../test_file4', 'test4', false); RESET ROLE; -REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,bool) FROM regress_user1; -REVOKE pg_read_all_settings FROM regress_user1; -DROP ROLE regress_user1; +REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,bool) FROM regress_adminpack_user1; +REVOKE pg_read_all_settings FROM regress_adminpack_user1; +DROP ROLE regress_adminpack_user1; -- sync SELECT pg_file_sync('test_file1'); -- sync file @@ -59,8 +59,8 @@ SELECT pg_file_unlink('test_file4'); -- superuser checks -CREATE USER regress_user1; -SET ROLE regress_user1; +CREATE USER regress_adminpack_user1; +SET ROLE regress_adminpack_user1; SELECT pg_file_write('test_file0', 'test0', false); SELECT pg_file_sync('test_file0'); @@ -69,7 +69,7 @@ SELECT pg_file_unlink('test_file0'); SELECT pg_logdir_ls(); RESET ROLE; -DROP USER regress_user1; +DROP USER regress_adminpack_user1; -- no further tests for pg_logdir_ls() because it depends on the diff --git a/contrib/passwordcheck/expected/passwordcheck.out b/contrib/passwordcheck/expected/passwordcheck.out index e04cda6bd95..2f917169eb6 100644 --- a/contrib/passwordcheck/expected/passwordcheck.out +++ b/contrib/passwordcheck/expected/passwordcheck.out @@ -1,19 +1,19 @@ LOAD 'passwordcheck'; -CREATE USER regress_user1; +CREATE USER regress_passwordcheck_user1; -- ok -ALTER USER regress_user1 PASSWORD 'a_nice_long_password'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'a_nice_long_password'; -- error: too short -ALTER USER regress_user1 PASSWORD 'tooshrt'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'tooshrt'; ERROR: password is too short -- error: contains user name -ALTER USER regress_user1 PASSWORD 'xyzregress_user1'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'xyzregress_passwordcheck_user1'; ERROR: password must not contain user name -- error: contains only letters -ALTER USER regress_user1 PASSWORD 'alessnicelongpassword'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'alessnicelongpassword'; ERROR: password must contain both letters and nonletters -- encrypted ok (password is "secret") -ALTER USER regress_user1 PASSWORD 'md51a44d829a20a23eac686d9f0d258af13'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'md51a44d829a20a23eac686d9f0d258af13'; -- error: password is user name -ALTER USER regress_user1 PASSWORD 'md5e589150ae7d28f93333afae92b36ef48'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'md507a112732ed9f2087fa90b192d44e358'; ERROR: password must not equal user name -DROP USER regress_user1; +DROP USER regress_passwordcheck_user1; diff --git a/contrib/passwordcheck/sql/passwordcheck.sql b/contrib/passwordcheck/sql/passwordcheck.sql index d98796ac494..df0f9ac0d39 100644 --- a/contrib/passwordcheck/sql/passwordcheck.sql +++ b/contrib/passwordcheck/sql/passwordcheck.sql @@ -1,23 +1,23 @@ LOAD 'passwordcheck'; -CREATE USER regress_user1; +CREATE USER regress_passwordcheck_user1; -- ok -ALTER USER regress_user1 PASSWORD 'a_nice_long_password'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'a_nice_long_password'; -- error: too short -ALTER USER regress_user1 PASSWORD 'tooshrt'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'tooshrt'; -- error: contains user name -ALTER USER regress_user1 PASSWORD 'xyzregress_user1'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'xyzregress_passwordcheck_user1'; -- error: contains only letters -ALTER USER regress_user1 PASSWORD 'alessnicelongpassword'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'alessnicelongpassword'; -- encrypted ok (password is "secret") -ALTER USER regress_user1 PASSWORD 'md51a44d829a20a23eac686d9f0d258af13'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'md51a44d829a20a23eac686d9f0d258af13'; -- error: password is user name -ALTER USER regress_user1 PASSWORD 'md5e589150ae7d28f93333afae92b36ef48'; +ALTER USER regress_passwordcheck_user1 PASSWORD 'md507a112732ed9f2087fa90b192d44e358'; -DROP USER regress_user1; +DROP USER regress_passwordcheck_user1; -- 2.37.3.542.gdd3f6c4cae --ngschxobz5ssor62 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v16-0016-meson-Add-installcheck-equivalent.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Memory leak in WAL sender with pgoutput (v10~) @ 2024-11-29 02:05 Michael Paquier <[email protected]> 2024-11-30 03:58 ` Re: Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Michael Paquier @ 2024-11-29 02:05 UTC (permalink / raw) To: Postgres hackers <[email protected]> Hi all, This is a follow-up of ea792bfd93ab and this thread where I've noticed that some memory was still leaking when running sysbench with a logical replication setup: https://www.postgresql.org/message-id/[email protected] Reproducing the problem is quite simple, and can be done with the following steps (please adapt, like previous thread): 1) Initialize a primary and set up some empty tables: NUM_TABLES=500 PRIMARY_PORT=5432 STANDBY_PORT=5433 sysbench oltp_read_only --db-driver=pgsql \ --pgsql-port=$PRIMARY_PORT \ --pgsql-db=postgres \ --pgsql-user=postgres \ --pgsql-host=/tmp \ --tables=$NUM_TABLES --table_size=0 \ --report-interval=1 \ --threads=1 prepare 2) Create a standby, promote it: STANDBY_DATA=/define/your/standby/path/ pg_basebackup --checkpoint=fast -D $STANDBY_DATA -p $PRIMARY_PORT -h /tmp/ -R echo "port = $STANDBY_PORT" >> $STANDBY_DATA/postgresql.conf pg_ctl -D $STANDBY_DATA start pg_ctl -D $STANDBY_DATA promote 3) Publication and subscription psql -p $PRIMARY_PORT -c "CREATE PUBLICATION pub1 FOR ALL TABLES;" psql -p $STANDBY_PORT -c "CREATE SUBSCRIPTION sub1 CONNECTION 'port=$PRIMARY_PORT user=postgres dbname=postgres PUBLICATION pub1 WITH (enabled, create_slot, slot_name='pub1_to_sub1', copy_data=false);" 4) Run sysbench: sysbench oltp_write_only --db-driver=pgsql \ --pgsql-port=$PRIMARY_PORT \ --pgsql-db=postgres \ --pgsql-user=postgres \ --pgsql-host=/tmp \ --tables=$NUM_TABLES --table_size=100 \ --report-interval=1 \ --threads=$NUM_THREADS run \ --time=5000 With that, I've mentioned internally that there was an extra leak and left the problem lying aside for a few days before being able to come back to it. In-between, Jeff Davis has done a review of the code to note that we leak memory in the CacheMemoryContext, due to the list_free_deep() done in get_rel_sync_entry() that misses the fact that the publication name is pstrdup()'d in GetPublication(), but list_free_deep() does not know that so the memory of the strdupped publication names stays around. That's wrong. Back to today, I've done more benchmarking using the above steps and logged periodic memory samples of the WAL sender with pg_log_backend_memory_contexts() (100s, 50 points), and analyzed the evolution of the whole thing. "Grand Total" used memory keeps increasing due to one memory context: CacheMemoryContext. So yes, Jeff has spotted what looks like the only leak we have. Attached is a graph that shows the evolution of memory between HEAD and a patched version for the used memory of CacheMemoryContext (ylabel is in bytes, could not get that right as my gnuplot skills are bad). One thing to note in this case is that I've made the leak more noticeable with this tweak to force the publication to be reset each time with go through get_rel_sync_entry(), or memory takes much longer to pile up. That does not change the problem, speeds up the detection by a large factor: --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -2061,12 +2061,13 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) List *rel_publications = NIL; /* Reload publications if needed before use. */ - if (!publications_valid) + elog(WARNING, "forcing reload publications"); { oldctx = MemoryContextSwitchTo(CacheMemoryContext); if (data->publications) { list_free_deep(data->publications); This problem exists since the introduction of logical replication, down to v10. It would be sad to have an extra loop on publications to free explicitely the publication names, and the issue would still be hidden to the existing callers of GetPublication(), so I'd suggest to change the Publication structure to use a NAMEDATALEN string to force the free to happen, as in the attached. That means an ABI break. I've looked around and did not notice any out-of-core code relying on sizeof(Publication). That does not mean that nothing would break, of course, but the odds should be pretty good as this is a rather internal structure. One way would be to just fix that on HEAD and call it a day, but I'm aware of heavy logirep users whose workloads would be able to see memory bloating because they maintain WAL senders around. Thoughts or comments? -- Michael Attachments: [text/csv] all_used_total.csv (1.0K, ../../[email protected]/2-all_used_total.csv) download | inline: 0,3968872,4014248 100,4011832,4014248 200,3997928,4001048 300,4022224,4001048 400,4040608,3984520 500,4042552,3999448 600,4053576,3999448 700,4067464,3938632 800,4068992,3998120 900,4080632,3998120 1000,4090056,3890312 1100,4083968,3998120 1200,4106728,3998120 1300,4119200,3949688 1400,4104680,3998120 1500,4134536,3998120 1600,4144408,3905128 1700,4137608,3998120 1800,4159744,3998120 1900,4171488,3952232 2000,4170472,3998120 2100,4186584,3998120 2200,4196520,3946040 2300,4192296,3998120 2400,4211712,3998120 2500,4221792,3916296 2600,4214752,3998120 2700,4237344,3998120 2800,4245656,3916184 2900,4244736,3998120 3000,4260920,3998120 3100,4268488,3937304 3200,4281000,3998120 3300,4285752,3998120 3400,4296144,3903688 3500,4294672,3998120 3600,4310760,3996904 3700,4320624,3945928 3800,4313824,3996904 3900,4335768,3998120 4000,4345776,3940952 4100,4355952,3998120 4200,4360992,3998120 4300,4370616,3898712 4400,4360504,3998120 4500,4385616,3998120 4600,4394168,3952120 4700,4394104,3998120 4800,4408280,3998120 4900,4418976,3931000 [image/png] memused_vs_time.png (29.6K, ../../[email protected]/3-memused_vs_time.png) download | view image [text/x-diff] logirep_cache_leak.patch (997B, ../../[email protected]/4-logirep_cache_leak.patch) download | inline diff: diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 9a83a72d6b..809bd1f0a6 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -103,7 +103,7 @@ typedef struct PublicationDesc typedef struct Publication { Oid oid; - char *name; + char name[NAMEDATALEN]; bool alltables; bool pubviaroot; bool pubgencols; diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 9bbb60463f..8f4e3b83f1 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -1061,7 +1061,7 @@ GetPublication(Oid pubid) pub = (Publication *) palloc(sizeof(Publication)); pub->oid = pubid; - pub->name = pstrdup(NameStr(pubform->pubname)); + strlcpy(pub->name, NameStr(pubform->pubname), NAMEDATALEN); pub->alltables = pubform->puballtables; pub->pubactions.pubinsert = pubform->pubinsert; pub->pubactions.pubupdate = pubform->pubupdate; [application/pgp-signature] signature.asc (833B, ../../[email protected]/5-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Memory leak in WAL sender with pgoutput (v10~) 2024-11-29 02:05 Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> @ 2024-11-30 03:58 ` Michael Paquier <[email protected]> 2024-11-30 08:28 ` Re: Memory leak in WAL sender with pgoutput (v10~) Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Michael Paquier @ 2024-11-30 03:58 UTC (permalink / raw) To: Postgres hackers <[email protected]>; +Cc: Amit Kapila <[email protected]> On Fri, Nov 29, 2024 at 11:05:51AM +0900, Michael Paquier wrote: > This is a follow-up of ea792bfd93ab and this thread where I've noticed > that some memory was still leaking when running sysbench with a > logical replication setup: > https://www.postgresql.org/message-id/[email protected] Adding Amit in CC in case, as we've talked about this topic offlist. > This problem exists since the introduction of logical replication, > down to v10. It would be sad to have an extra loop on publications to > free explicitely the publication names, and the issue would still be > hidden to the existing callers of GetPublication(), so I'd suggest to > change the Publication structure to use a NAMEDATALEN string to force > the free to happen, as in the attached. > > That means an ABI break. I've looked around and did not notice any > out-of-core code relying on sizeof(Publication). That does not mean > that nothing would break, of course, but the odds should be pretty > good as this is a rather internal structure. One way would be to just > fix that on HEAD and call it a day, but I'm aware of heavy logirep > users whose workloads would be able to see memory bloating because > they maintain WAL senders around. After sleeping on that, and because the leak is minimal, I'm thinking about just applying the fix only on HEAD and call it a day. This changes the structure of Publication so as we use a char[NAMEDATALEN] rather than a char*, avoiding the pstrdup(), for the publication name and free it in the list_free_deep() when reloading the list of publications. If there are any objections or comments, feel free. I'll revisit that again around the middle of next week. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Memory leak in WAL sender with pgoutput (v10~) 2024-11-29 02:05 Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> 2024-11-30 03:58 ` Re: Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> @ 2024-11-30 08:28 ` Alvaro Herrera <[email protected]> 2024-12-02 01:49 ` Re: Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Alvaro Herrera @ 2024-11-30 08:28 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Amit Kapila <[email protected]> On 2024-Nov-30, Michael Paquier wrote: > After sleeping on that, and because the leak is minimal, I'm thinking > about just applying the fix only on HEAD and call it a day. This > changes the structure of Publication so as we use a char[NAMEDATALEN] > rather than a char*, avoiding the pstrdup(), for the publication name > and free it in the list_free_deep() when reloading the list of > publications. I'm not sure about your proposed fix. Isn't it simpler to have another memory context which we can reset instead of doing list_free_deep()? It doesn't have to be a global memory context -- since this is not reentrant and not referenced anywhere else, it can be a simple static variable in that block, as in the attached. I ran the stock tests (no sysbench) and at least it doesn't crash. This should be easily backpatchable also, since there's no ABI change. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ Attachments: [text/x-diff] 0001-untested-try-at-fixing-memleak.patch (1.3K, ../../[email protected]/2-0001-untested-try-at-fixing-memleak.patch) download | inline diff: From 3ed287180a007447bbee152177139cf1d4347625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Sat, 30 Nov 2024 09:02:10 +0100 Subject: [PATCH] untested try at fixing memleak --- src/backend/replication/pgoutput/pgoutput.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 5e23453f071..010b341e0f5 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -2063,12 +2063,16 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) /* Reload publications if needed before use. */ if (!publications_valid) { - oldctx = MemoryContextSwitchTo(CacheMemoryContext); - if (data->publications) - { - list_free_deep(data->publications); - data->publications = NIL; - } + static MemoryContext pubmemcxt = NULL; + + if (pubmemcxt == NULL) + pubmemcxt = AllocSetContextCreate(CacheMemoryContext, + "publication list context", + ALLOCSET_DEFAULT_SIZES); + else + MemoryContextReset(pubmemcxt); + oldctx = MemoryContextSwitchTo(pubmemcxt); + data->publications = LoadPublications(data->publication_names); MemoryContextSwitchTo(oldctx); publications_valid = true; -- 2.39.5 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Memory leak in WAL sender with pgoutput (v10~) 2024-11-29 02:05 Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> 2024-11-30 03:58 ` Re: Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> 2024-11-30 08:28 ` Re: Memory leak in WAL sender with pgoutput (v10~) Alvaro Herrera <[email protected]> @ 2024-12-02 01:49 ` Michael Paquier <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Michael Paquier @ 2024-12-02 01:49 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Postgres hackers <[email protected]>; Amit Kapila <[email protected]> On Sat, Nov 30, 2024 at 09:28:31AM +0100, Alvaro Herrera wrote: > I'm not sure about your proposed fix. Isn't it simpler to have another > memory context which we can reset instead of doing list_free_deep()? It > doesn't have to be a global memory context -- since this is not > reentrant and not referenced anywhere else, it can be a simple static > variable in that block, as in the attached. I ran the stock tests (no > sysbench) and at least it doesn't crash. > > This should be easily backpatchable also, since there's no ABI change. Yeah. Using more memory contexts around these API calls done without the cache manipulation is something I've also mentioned to Amit a few days ago, and I'm feeling that this concept should be applied to a broader area than just the cached publication list in light of this thread and ea792bfd93ab. That's a discussion for later, likely. I'm not sure how broad this should be and if this should be redesign to make the whole context tracking easier. What I am sure of at this stage is that for this workload we don't have any garbage left behind. > + static MemoryContext pubmemcxt = NULL; > + > + if (pubmemcxt == NULL) > + pubmemcxt = AllocSetContextCreate(CacheMemoryContext, > + "publication list context", > + ALLOCSET_DEFAULT_SIZES); > + else > + MemoryContextReset(pubmemcxt); > + oldctx = MemoryContextSwitchTo(pubmemcxt); > + > data->publications = LoadPublications(data->publication_names); > MemoryContextSwitchTo(oldctx); > publications_valid = true; Something like that works for me in stable branches, removing the need for the list free as there is only one caller of LoadPublications() currently. I've checked with my previous script, with the aggressive invalidation tweak, in the case I'm missing something, and the memory numbers are stable. I am slightly concerned about the current design of GetPublication() in the long-term, TBH. LoadPublications() has hidden the leak behind two layers of routines in the WAL sender, and that's easy to miss once you call anything that loads a Publication depending on how the caller caches its data. So I would still choose for modifying the structure on HEAD removing the pstrdup() for the publication name. Anyway, your suggestion is also OK by me on top of that, that's less conflicts in all the branches. May I suggest the attached then? 0001 is your backpatch, 0002 would be my HEAD-only suggestion, if that's OK for you and others of course. -- Michael Attachments: [text/x-diff] v2-0001-Fix-memory-leak-in-pgoutput-with-publication-list.patch (1.8K, ../../[email protected]/2-v2-0001-Fix-memory-leak-in-pgoutput-with-publication-list.patch) download | inline diff: From 22795bd2c7a8c7934d4ac77cfe1cd2ef36ae9fd3 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 2 Dec 2024 09:50:01 +0900 Subject: [PATCH v2 1/2] Fix memory leak in pgoutput with publication list cache Blah. Analyzed-by: Jeff Davis, Michael Paquier Author: Alvaro Herrera Discussion: https://postgr.es/m/[email protected] Backpatch-through: 13 --- src/backend/replication/pgoutput/pgoutput.c | 22 ++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 5e23453f07..41d204ac3b 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -2060,15 +2060,23 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) char relkind = get_rel_relkind(relid); List *rel_publications = NIL; - /* Reload publications if needed before use. */ + /* + * Reload publications if needed before use. A dedicated memory + * context is used to hold the publication information, so as any + * memory allocated within LoadPublications() is freed. + */ if (!publications_valid) { - oldctx = MemoryContextSwitchTo(CacheMemoryContext); - if (data->publications) - { - list_free_deep(data->publications); - data->publications = NIL; - } + static MemoryContext pubmemcxt = NULL; + + if (pubmemcxt == NULL) + pubmemcxt = AllocSetContextCreate(CacheMemoryContext, + "publication list context", + ALLOCSET_DEFAULT_SIZES); + else + MemoryContextReset(pubmemcxt); + oldctx = MemoryContextSwitchTo(pubmemcxt); + data->publications = LoadPublications(data->publication_names); MemoryContextSwitchTo(oldctx); publications_valid = true; -- 2.45.2 [text/x-diff] v2-0002-Avoid-extra-pstrdup-in-Publication.patch (1.5K, ../../[email protected]/3-v2-0002-Avoid-extra-pstrdup-in-Publication.patch) download | inline diff: From 22cb03ab8e0fe024f8c3bb36b1581a32eff04ebc Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 2 Dec 2024 10:36:52 +0900 Subject: [PATCH v2 2/2] Avoid extra pstrdup() in Publication The structure is changed so as the publication name is included in a single memory allocation, knowing that the publication name is limited by NAMEDATALEN. No backpatch is required, as this is an ABI breakage. --- src/include/catalog/pg_publication.h | 2 +- src/backend/catalog/pg_publication.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 9a83a72d6b..809bd1f0a6 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -103,7 +103,7 @@ typedef struct PublicationDesc typedef struct Publication { Oid oid; - char *name; + char name[NAMEDATALEN]; bool alltables; bool pubviaroot; bool pubgencols; diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 9bbb60463f..8f4e3b83f1 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -1061,7 +1061,7 @@ GetPublication(Oid pubid) pub = (Publication *) palloc(sizeof(Publication)); pub->oid = pubid; - pub->name = pstrdup(NameStr(pubform->pubname)); + strlcpy(pub->name, NameStr(pubform->pubname), NAMEDATALEN); pub->alltables = pubform->puballtables; pub->pubactions.pubinsert = pubform->pubinsert; pub->pubactions.pubupdate = pubform->pubupdate; -- 2.45.2 [application/pgp-signature] signature.asc (833B, ../../[email protected]/4-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-12-02 01:49 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-09-25 23:49 [PATCH v16 15/16] tests: Rename conflicting role names Andres Freund <[email protected]> 2024-11-29 02:05 Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> 2024-11-30 03:58 ` Re: Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[email protected]> 2024-11-30 08:28 ` Re: Memory leak in WAL sender with pgoutput (v10~) Alvaro Herrera <[email protected]> 2024-12-02 01:49 ` Re: Memory leak in WAL sender with pgoutput (v10~) Michael Paquier <[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