Обсуждение: [PATCH] Fix escaping for '\' and '"' in pageinspect for gist
Hi hackers! I noticed, that there is bug in escaping values that contains '\' or '"' in text representation inside pageinspect for gist: the string 'foo"bar' are printed like "foo""bar" and not "foo\"bar". To fix that, we should do appendStringInfoCharMacro(&buf, '\\'); instead of appendStringInfoCharMacro(&buf, ch); in case ch is one of that symbols. Any thoughts? -- Best regards, Roman Khapov
Вложения
On Mon, 29 Dec 2025 at 20:48, Roman Khapov <rkhapov@yandex-team.ru> wrote: > > Hi hackers! > > I noticed, that there is bug in escaping values that contains '\' or '"' in text representation > inside pageinspect for gist: the string 'foo"bar' are printed like "foo""bar" and not "foo\"bar". > > To fix that, we should do appendStringInfoCharMacro(&buf, '\\'); instead of > appendStringInfoCharMacro(&buf, ch); in case ch is one of that symbols. > > Any thoughts? > > -- > Best regards, > Roman Khapov > Hi! Looks like we have two similar places in kernel [0] & [1], and at least one of these three places is wrong. [0] https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/rowtypes.c#L460 [1] https://github.com/postgres/postgres/blob/master/src/bin/pg_rewind/libpq_source.c#L621 -- Best regards, Kirill Reshke
Roman Khapov <rkhapov@yandex-team.ru> writes:
> I noticed, that there is bug in escaping values that contains '\' or '"' in text representation
> inside pageinspect for gist: the string 'foo"bar' are printed like "foo""bar" and not "foo\"bar".
I do not think this is a bug. The comment at line 295 says
"Most of this is copied from record_out().", and this logic
matches what record_out() does, and the output is legal
according to the manual's specifications [1]:
To put a double quote or backslash in a quoted composite field
value, precede it with a backslash. (Also, a pair of double quotes
within a double-quoted field value is taken to represent a double
quote character, analogously to the rules for single quotes in SQL
literal strings.)
Now, your alternative coding would also produce legal output, but
I do not think unnecessary change here is a good thing.
regards, tom lane
[1] https://www.postgresql.org/docs/devel/rowtypes.html#ROWTYPES-IO-SYNTAX
On Mon, 29 Dec 2025 at 23:37, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Roman Khapov <rkhapov@yandex-team.ru> writes: > > I noticed, that there is bug in escaping values that contains '\' or '"' in text representation > > inside pageinspect for gist: the string 'foo"bar' are printed like "foo""bar" and not "foo\"bar". > > I do not think this is a bug. The comment at line 295 says > "Most of this is copied from record_out().", and this logic > matches what record_out() does, and the output is legal > according to the manual's specifications [1]: > > To put a double quote or backslash in a quoted composite field > value, precede it with a backslash. (Also, a pair of double quotes > within a double-quoted field value is taken to represent a double > quote character, analogously to the rules for single quotes in SQL > literal strings.) > > Now, your alternative coding would also produce legal output, but > I do not think unnecessary change here is a good thing. > > regards, tom lane > > [1] https://www.postgresql.org/docs/devel/rowtypes.html#ROWTYPES-IO-SYNTAX > > Should we then refactor code to avoid copying? I copied this code in [0] for the third time, so if this has a chance to be committed, there will be 3 times copied code... [0] https://www.postgresql.org/message-id/CALdSSPgpD5RfPn5qMbozU4_SQpZAbG3V_%3DKdxV9YaEG9gX%3DqEA%40mail.gmail.com -- Best regards, Kirill Reshke
Kirill Reshke <reshkekirill@gmail.com> writes:
> Should we then refactor code to avoid copying? I copied this code in
> [0] for the third time, so if this has a chance to be committed, there
> will be 3 times copied code...
Might not be a bad idea, but of course the devil is in the details.
What did you have in mind? Split out a function to quote one
field value per record_out's rules?
regards, tom lane
On Tue, 30 Dec 2025 at 00:24, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Kirill Reshke <reshkekirill@gmail.com> writes: > > Should we then refactor code to avoid copying? I copied this code in > > [0] for the third time, so if this has a chance to be committed, there > > will be 3 times copied code... > > Might not be a bad idea, but of course the devil is in the details. > What did you have in mind? Split out a function to quote one > field value per record_out's rules? > > regards, tom lane Yes, split out a function that does quoting after OidOutputFunctionCall. that is, this: https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/rowtypes.c#L437-L464 -- Best regards, Kirill Reshke
On Tue, Dec 30, 2025 at 12:29:49AM +0500, Kirill Reshke wrote: > Yes, split out a function that does quoting after > OidOutputFunctionCall. that is, this: > > https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/rowtypes.c#L437-L464 FWIW, refactoring that into a single function is something I have considered while working on e7bff46e50b8, as something that could be done on HEAD. At the end, I could not get excited with the result I got (see the end of the thread) for the amount of lines saved, also because this impacts a limited area of the code with what seems like a reduction in readability. -- Michael