John Naylor <jcnaylor@gmail.com> writes:
> On 4/5/18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I did not like the hard-wired handling of proargtypes and proallargtypes
>> in genbki.pl; it hardly seems impossible that we'll want similar
>> conversions for other array-of-OID columns in future. After a bit of
>> thought, it seemed like we could allow
>> oidvector proargtypes BKI_LOOKUP(pg_type);
>> Oid proallargtypes[1] BKI_DEFAULT(_null_) BKI_LOOKUP(pg_type);
>> and just teach genbki.pl that if a lookup rule is attached to
>> an oidvector or Oid[] column, it means to apply the rule to
>> each array element individually.
> I think that's a good idea. I went an extra step and extracted the
> common logic into a function (attached draft patch to be applied on
> top of yours). It treats all lookups as operating on arrays. The
> common case is that we pass a single-element array. That may seem
> awkward, but I think it's clear. The code is slimmer, and the lines
> now fit within 80 characters.
Sounds good. I too was bothered by the duplication of code, but
I'm not a good enough Perl programmer to have thought of that solution.
Something that bothered me a bit while writing the warning-producing code
is that showing %bki_values isn't actually that great a way of identifying
the trouble spot. By this point we've expanded out defaults and possibly
replaced some other macros, so it doesn't look that much like what was
in the .dat file. I think what would be ideal, both here and in some
other places like AddDefaultValues, is to be able to finger the location
of the bad tuple by filename and line number, but I have no idea whether
it's practical to annotate the tuples with that while reading the .dat
files. Any thoughts?
(Obviously, better error messages could be a future improvement; it's not
something we have to get done before the conversion.)
> Unrelated, I noticed my quoting of defaults that contain back-slashes
> was half-baked, so I'll include that fix in the next patchset. I'll
> put out a new one in a couple days, to give a chance for further
> review and discussion of the defaults. I didn't feel the need to
> respond to the other messages, but yours and Andres' points are well
> taken.
We're getting down to the wire here --- I think the plan is to close
the CF on Saturday or Sunday, and then push the bootstrap changes right
after that. So please turn around whatever you're planning to do ASAP.
I'm buckling down to a final review today and tomorrow.
regards, tom lane