Re: Add absolute value to dict_int

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Jeff Janes <jeff(dot)janes(at)gmail(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Add absolute value to dict_int
Date: 2020-03-09 23:49:53
Message-ID: 21875.1583797793@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> There are at least three things we could do here:
> 1. Insist that defGetBoolean and its siblings should accept the
> string equivalent of any non-string value they accept. This
> would change the behavior of a whole lot of utility commands,
> not only the text search commands, and I'm not exactly convinced
> it's a good idea. Seems like it's losing error detection
> capability.
> 2. Improve the catalog representation of text search parameters
> so that the original Value node can be faithfully reconstructed.
> I'd be for this, except it seems like a lot of work for a rather
> minor benefit.
> 3. Rearrange text search parameter validation so we smash parameters
> to strings *before* we validate them, ensuring we won't take any
> settings that will be rejected later.

I still don't much like #1, but after looking closer, #2 is not as
impractical as I thought. The catalog representation doesn't need
any redefinition really, because per the existing comments,

* For the convenience of pg_dump, the output is formatted exactly as it
* would need to appear in CREATE TEXT SEARCH DICTIONARY to reproduce the
* same options.

So all we really need to do is upgrade [de]serialize_deflist to be smarter
about int and float nodes. This is still a bit invasive because somebody
decided to make deserialize_deflist serve two masters, and I don't feel
like working through whether the prsheadline code would cope nicely with
non-string output nodes. So the first patch attached adds a flag argument
to deserialize_deflist to tell it whether to force all the values to
strings.

Alternatively, we could do #3, as in the second patch below. This
seems clearly Less Good, but it's a smaller/safer patch, and it's
at least potentially back-patchable, whereas changing the signature
of deserialize_deflist in stable branches would risk trouble.

(I didn't bother with regression test additions yet, but some would
be appropriate for either patch.)

Given the lack of field complaints, I'm not that excited about
back-patching anything for this. So my inclination is to go with #2
(first patch) and fix it in HEAD only.

Thoughts?

regards, tom lane

Attachment Content-Type Size
fix-serialization.patch text/x-diff 7.8 KB
deserialize-reserialize.patch text/x-diff 3.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2020-03-10 00:06:55 Re: Nicer error when connecting to standby with hot_standby=off
Previous Message Thomas Munro 2020-03-09 23:20:31 Re: effective_io_concurrency's steampunk spindle maths