Re: log bind parameter values on error

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> I added some tests to the pgbench suite in the attached. I also finally
> put it the business to limit the length of parameters reported.
> I'd probably drop testlibpq5.c, since it seems a bit pointless now ...

I took a look through the v17 patches.

0001:

The two header comments for appendStringInfoStringQuoted really ought to
define the maxlen parameter, rather than making readers reverse-engineer
what that does.

I'm not very sold on the enlargeStringInfo call in the maxlen>0 code path,
mainly because its calculation of the needed space seems entirely wrong:
(a) it's considering maxlen not the actual length, and (b) on the other
hand it's not accounting for quotes and ellipses. Forcing an inadequate
enlargement is probably worse than doing nothing, so I'd be inclined to
just drop that.

It is a very bad idea that this is truncating text without regard to
multibyte character boundaries.

0002:

Seems like BuildParamLogString's "valueLen" parameter ought to be called
"maxlen", for consistency with 0001 and because "valueLen" is at best
misleading about what the parameter means.

I'd toss the enlargeStringInfo call here too, as it seems overly
complicated and underly correct or useful.

Probably doing MemoryContextReset after each parameter (even the last one!)
is a net loss compared to just letting it go till the end. Although
I'd be inclined to use ALLOCSET_DEFAULT_SIZES not SMALL_SIZES if you
do it like that.

Please do not throw away the existing comment "/* Free result of encoding
conversion, if any */" in exec_bind_message.

As above, this risks generating partial multibyte characters. You might
be able to get away with letting appendStringInfoStringQuoted do the
multibyte-aware truncation, but you probably have to copy more than just
one more extra byte first.

I have zero faith in this:

+ params_errcxt.arg = (void *) &((ParamsErrorCbData)
+ { portal->name, params });

How do you know where the compiler is putting that value, ie what
its lifespan is going to be? Better to use an explicit variable.

I concur with dropping testlibpq5.

regards, tom lane