Re: inconsistency and inefficiency in setup_conversion()

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