On Tue, 10 Oct 2023 at 06:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hm. I'd be inclined to use maxlen == 0 as the indicator of a read-only
> buffer, just because that would not create a problem if we ever want
> to change it to an unsigned type. Other than that, I agree with the
> idea of using a special maxlen value to indicate that the buffer is
> read-only and not owned by the StringInfo. We need to nail down the
> exact semantics though.
I've attached a slightly more worked on patch that makes maxlen == 0
mean read-only. Unsure if a macro is worthwhile there or not.
The patch still fails during 023_twophase_stream.pl for the reasons
mentioned upthread. Getting rid of the Assert in
initStringInfoFromStringWithLen() allows it to pass.
One thought I had about this is that the memory context behaviour
might catch someone out at some point. Right now if you do
initStringInfo() the memory context of the "data" field will be
CurrentMemoryContext, but if someone does
initStringInfoFromStringWithLen() and then changes to some other
memory context before doing an appendStringInfo on that string, then
we'll allocate "data" in whatever that memory context is. Maybe that's
ok if we document it. Fixing it would mean adding a MemoryContext
field to StringInfoData which would be set to CurrentMemoryContext
during initStringInfo() and initStringInfoFromStringWithLen().
I'm not fully happy with the extra code added in enlargeStringInfo().
It's a little repetitive. Fixing it up would mean having to have a
boolean variable to mark if the string was readonly so at the end we'd
know to repalloc or palloc/memcpy. For now, I just marked that code
as unlikely() since there's no place in the code base that uses it.
David