[PATCH] Fix escaping for '\' and '"' in pageinspect for gist
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t52986psql -h localhost -U postgresBuilt from patchset v1 (message #1), September 20, 2026 at 02:06 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t52986_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t52986_1 && git checkout t52986_1Patchset v1 (message #1) is on t52986_1
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]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, 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
--
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]https://www.postgresql.org/docs/devel/rowtypes.html#ROWTYPES-IO-SYNTAX:
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]: /messages/by-id/CALdSSPgpD5RfPn5qMbozU4_SQpZAbG3V_=KdxV9YaEG9gX=qEA@mail.gmail.com
will be 3 times copied code...
[0]: /messages/by-id/CALdSSPgpD5RfPn5qMbozU4_SQpZAbG3V_=KdxV9YaEG9gX=qEA@mail.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
Import Notes
Reply to msg id not found: 2993271.1767036289@sss.pgh.pa.us
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