From: | John Naylor <jcnaylor(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: inconsistency and inefficiency in setup_conversion() |
Date: | 2018-05-02 12:19:51 |
Message-ID: | CAJVSVGXmARopxgpWTg12MD=2C2q0h1J2bH9uzVONTmkaPcjoLA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 4/28/18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> John Naylor <jcnaylor(at)gmail(dot)com> writes:
>> Solution #1 - As alluded to in [1], turn the conversions into
>> pg_proc.dat and pg_conversion.dat entries. Teach genbki.pl to parse
>> pg_wchar.h to map conversion names to numbers.
>> Pros:
>> -likely easy to do
>> -allows for the removal of an install target in the Makefile as well
>> as ad hoc logic in MSVC
>> -uses a format that developers need to use anyway
>> Cons:
>> -immediately burns up 88 hard-coded OIDs and one for each time a
>> conversion proc is created
>> -would require editing data in two catalogs every time a conversion
>> proc is created
>
> Given the rate at which new conversion procs have been created
> historically (ie, next door to zero, after the initial feature addition),
> I don't think that second "con" argument has any force. Eating a batch
> of manually-assigned OIDs seems risky mainly just in that it might force
> adjustment of pending patches --- but we deal with that all the time.
> So I like this answer, I think.
Attached is a draft patch to do this, along with the conversion script
used to create the entries. In writing this, a few points came up that
are worth bringing up:
-In the original SQL file the functions were not declared with an
explicit volatility, so by default they are 'volatile'. That seems
wrong for this kind of function, so I changed it to 'immutable'. It
seems the CREATE CONVERSION facility was created shortly after the
volatility classes were created, and I couldn't find any discussion
about it.
-I have not done performance testing of initdb yet. I'll do so at a
later date unless someone is excited enough to beat me to it.
-I piggy-backed on the OID lookup machinery for the encoding lookup,
but haven't changed all the comments that refer only to catalogs and
OIDs.
-With the 88 pg_proc entries with prolang=13 along with the 50 or so
with prolang=14, it might be worth it to create a language lookup.
This patch does not do so, however.
-This actually uses up 220 OIDs (88 + 132), since the conversions need
them for their comments to be loaded.
> However, there is a "con" you didn't mention that perhaps ought to be
> accounted for. The way things are done now, neither these C functions
> nor the pg_conversion entries are "pinned"; it's possible to drop and/or
> recreate them. That perhaps had significant value during development
> of the conversions feature, but I'm doubtful that it's worth much
> anymore. Still, it's worth pointing out in case somebody disagrees.
-For this draft, I let them get pinned, and changed the sanity test to
reflect that. It'd be easy enough to add exceptions to setup_depend(),
though. (one for pg_conversion, and one to change the pg_proc query to
exclude C language functions)
I'll create a commitfest entry soon.
-John Naylor
Attachment | Content-Type | Size |
---|---|---|
v1-0001-Replace-ad-hoc-format-for-conversion-functions.patch | text/x-patch | 72.2 KB |
v1-convert_conversion2dat.pl | application/x-perl | 13.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Ashutosh Bapat | 2018-05-02 13:17:23 | Re: Remove mention in docs that foreign keys on partitioned tables are not supported |
Previous Message | Dmitry Dolgov | 2018-05-02 12:02:00 | Re: FPW stats? |