public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v1] Use stack allocated StringInfoDatas, where possible 4+ messages / 2 participants [nested] [flat]
* [PATCH v1] Use stack allocated StringInfoDatas, where possible @ 2026-04-12 10:09 Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Bertrand Drouvot @ 2026-04-12 10:09 UTC (permalink / raw) Commit 6d0eba66275 already did most of the changes but missed the opportunities in sequencesync.c. Indeed, 5509055d6956 added a few cases that are using StringInfo but don't need that StringInfo to exist beyond the scope of the function were using makeStringInfo(), which allocates both a StringInfoData and the buffer it uses as two separate allocations. It's more efficient for these cases to use a StringInfoData on the stack and initialize it with initStringInfo(), which only allocates the string buffer. The reason 6d0eba66275 missed those is that 5509055d6956 has been committed between the patch proposal for 6d0eba66275 and 6d0eba66275. Author: Bertrand Drouvot <[email protected]> --- .../replication/logical/sequencesync.c | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) 100.0% src/backend/replication/logical/ diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c index a4fb6783ba9..ec7e76abf93 100644 --- a/src/backend/replication/logical/sequencesync.c +++ b/src/backend/replication/logical/sequencesync.c @@ -174,45 +174,45 @@ static void report_sequence_errors(List *mismatched_seqs_idx, List *insuffperm_seqs_idx, List *missing_seqs_idx) { - StringInfo seqstr; + StringInfoData seqstr; /* Quick exit if there are no errors to report */ if (!mismatched_seqs_idx && !insuffperm_seqs_idx && !missing_seqs_idx) return; - seqstr = makeStringInfo(); + initStringInfo(&seqstr); if (mismatched_seqs_idx) { - get_sequences_string(mismatched_seqs_idx, seqstr); + get_sequences_string(mismatched_seqs_idx, &seqstr); ereport(WARNING, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg_plural("mismatched or renamed sequence on subscriber (%s)", "mismatched or renamed sequences on subscriber (%s)", list_length(mismatched_seqs_idx), - seqstr->data)); + seqstr.data)); } if (insuffperm_seqs_idx) { - get_sequences_string(insuffperm_seqs_idx, seqstr); + get_sequences_string(insuffperm_seqs_idx, &seqstr); ereport(WARNING, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg_plural("insufficient privileges on sequence (%s)", "insufficient privileges on sequences (%s)", list_length(insuffperm_seqs_idx), - seqstr->data)); + seqstr.data)); } if (missing_seqs_idx) { - get_sequences_string(missing_seqs_idx, seqstr); + get_sequences_string(missing_seqs_idx, &seqstr); ereport(WARNING, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg_plural("missing sequence on publisher (%s)", "missing sequences on publisher (%s)", list_length(missing_seqs_idx), - seqstr->data)); + seqstr.data)); } ereport(ERROR, @@ -388,10 +388,13 @@ copy_sequences(WalReceiverConn *conn) List *mismatched_seqs_idx = NIL; List *missing_seqs_idx = NIL; List *insuffperm_seqs_idx = NIL; - StringInfo seqstr = makeStringInfo(); - StringInfo cmd = makeStringInfo(); + StringInfoData seqstr; + StringInfoData cmd; MemoryContext oldctx; + initStringInfo(&seqstr); + initStringInfo(&cmd); + #define MAX_SEQUENCES_SYNC_PER_BATCH 100 elog(DEBUG1, @@ -423,13 +426,13 @@ copy_sequences(WalReceiverConn *conn) LogicalRepSequenceInfo *seqinfo = (LogicalRepSequenceInfo *) list_nth(seqinfos, idx); - if (seqstr->len > 0) - appendStringInfoString(seqstr, ", "); + if (seqstr.len > 0) + appendStringInfoString(&seqstr, ", "); nspname_literal = quote_literal_cstr(seqinfo->nspname); seqname_literal = quote_literal_cstr(seqinfo->seqname); - appendStringInfo(seqstr, "(%s, %s, %d)", + appendStringInfo(&seqstr, "(%s, %s, %d)", nspname_literal, seqname_literal, idx); if (++batch_size == MAX_SEQUENCES_SYNC_PER_BATCH) @@ -468,7 +471,7 @@ copy_sequences(WalReceiverConn *conn) * corresponding local entries without relying on result order or name * matching. */ - appendStringInfo(cmd, + appendStringInfo(&cmd, "SELECT s.seqidx, ps.*, seq.seqtypid,\n" " seq.seqstart, seq.seqincrement, seq.seqmin,\n" " seq.seqmax, seq.seqcycle\n" @@ -477,9 +480,9 @@ copy_sequences(WalReceiverConn *conn) "JOIN pg_class c ON c.relnamespace = n.oid AND c.relname = s.seqname\n" "JOIN pg_sequence seq ON seq.seqrelid = c.oid\n" "JOIN LATERAL pg_get_sequence_data(seq.seqrelid) AS ps ON true\n", - seqstr->data); + seqstr.data); - res = walrcv_exec(conn, cmd->data, lengthof(seqRow), seqRow); + res = walrcv_exec(conn, cmd.data, lengthof(seqRow), seqRow); if (res->status != WALRCV_OK_TUPLES) ereport(ERROR, errcode(ERRCODE_CONNECTION_FAILURE), @@ -567,8 +570,8 @@ copy_sequences(WalReceiverConn *conn) ExecDropSingleTupleTableSlot(slot); walrcv_clear_result(res); - resetStringInfo(seqstr); - resetStringInfo(cmd); + resetStringInfo(&seqstr); + resetStringInfo(&cmd); batch_missing_count = batch_size - (batch_succeeded_count + batch_mismatched_count + -- 2.34.1 --wggHK0nYArblB/da-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Use stack allocated StringInfoDatas, where possible (round 2) @ 2026-04-12 10:49 Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Bertrand Drouvot @ 2026-04-12 10:49 UTC (permalink / raw) To: [email protected]; +Cc: David Rowley <[email protected]> Hi hackers, Commit 6d0eba66275 already did most of the changes but missed the opportunities to $SUBJECT in sequencesync.c. Indeed, 5509055d6956 added a few cases that are using StringInfo but don't need that StringInfo to exist beyond the scope of the function were using makeStringInfo(), which allocates both a StringInfoData and the buffer it uses as two separate allocations. It's more efficient for these cases to use a StringInfoData on the stack and initialize it with initStringInfo(), which only allocates the string buffer. The reason 6d0eba66275 missed those is that 5509055d6956 has been committed between the patch proposal for 6d0eba66275 and 6d0eba66275. I used Mats's coccinelle script (mentioned in [1]) to find those and they are the only remaining ones. [1]: https://postgr.es/m/4379aac8-26f1-42f2-a356-ff0e886228d3%40gmail.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Use stack allocated StringInfoDatas, where possible (round 2) @ 2026-04-12 13:34 David Rowley <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: David Rowley @ 2026-04-12 13:34 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: [email protected] On Sun, 12 Apr 2026 at 22:49, Bertrand Drouvot <[email protected]> wrote: > Indeed, 5509055d6956 added a few cases that are using StringInfo but don't need > that StringInfo to exist beyond the scope of the function were using makeStringInfo(), > which allocates both a StringInfoData and the buffer it uses as two separate > allocations. It's more efficient for these cases to use a StringInfoData on the > stack and initialize it with initStringInfo(), which only allocates the string > buffer. I think the author of copy_sequences() doesn't know what resetStringInfo() does. I expect they think that it'll pfree all the memory, but that's what destroyStringInfo() is for. Since the strings are not used after the resetStringInfo(), the call is pointless. Could you ask Amit K to fix that first? Then rebase your patch atop. I think it makes sense to apply your patch as a StringInfo fixup post-freeze for v19 as leaving this for v20 will just result in the code not matching for no particularly good reason. David ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Use stack allocated StringInfoDatas, where possible (round 2) @ 2026-04-12 22:45 David Rowley <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: David Rowley @ 2026-04-12 22:45 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: [email protected] On Mon, 13 Apr 2026 at 01:34, David Rowley <[email protected]> wrote: > I think the author of copy_sequences() doesn't know what > resetStringInfo() does. I expect they think that it'll pfree all the > memory, but that's what destroyStringInfo() is for. That was poor analysis. The resetStringInfo is there to reset the StringInfo before the next loop. So nothing is wrong with it. I've now pushed your patch. David ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-04-12 22:45 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-04-12 10:09 [PATCH v1] Use stack allocated StringInfoDatas, where possible Bertrand Drouvot <[email protected]> 2026-04-12 10:49 Use stack allocated StringInfoDatas, where possible (round 2) Bertrand Drouvot <[email protected]> 2026-04-12 13:34 ` Re: Use stack allocated StringInfoDatas, where possible (round 2) David Rowley <[email protected]> 2026-04-12 22:45 ` Re: Use stack allocated StringInfoDatas, where possible (round 2) David Rowley <[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