Re: Compatible defaults for LEAD/LAG

út 22. 9. 2020 v 2:33 odesílatel Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> napsal:

> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> > I see few possibilities how to finish and close this issue:
> > 1. use anycompatible type and add note to documentation so expression of
> > optional argument can change a result type, and so this is Postgres
> > specific - other databases and ANSI SQL disallow this.
> > It is the most simple solution, and it is good enough for this case, that
> > is not extra important.
> > 2. choose the correct type somewhere inside the parser - for these two
> > functions.
> > 3. introduce and implement some generic solution for this case - it can
> be
> > implemented on C level via some function helper or on syntax level
> > CREATE OR REPLACE FUNCTION lag(a anyelement, offset int, default
> defval
> > a%type) ...
> > "defval argname%type" means for caller's expression "CAST(defval AS
> > typeof(argname))"
>
> I continue to feel that the spec's definition of this is not so
> obviously right that we should jump through hoops to duplicate it.
> In fact, I don't even agree that we need a disclaimer in the docs
> saying that it's not quite the same. Only the most nitpicky
> language lawyers would ever even notice.
>
> In hopes of moving this along a bit, I experimented with converting
> the other functions I listed to use anycompatible. I soon found that
> touching any functions/operators that are listed in operator classes
> would open a can of worms far larger than I'd previously supposed.
> To maintain consistency, we'd have to propagate the datatype changes
> to *every* function/operator associated with those opfamilies --- many
> of which only have one any-foo input and thus aren't on my original
> list. (An example here is that extending btree array_ops will require
> changing the max(anyarray) and min(anyarray) aggregates too.) We'd
> then end up with a situation that would be rather confusing from a
> user's standpoint, in that it'd be quite un-obvious why some array
> functions use anyarray while other ones use anycompatiblearray.
>
> So I backed off to just changing the functions/operators that have
> no opclass connections, such as array_cat. Even that has some
> downsides --- for example, in the attached patch, it's necessary
> to change some polymorphism.sql examples that explicitly reference
> array_cat(anyarray). I wonder whether this change would break a
> significant number of user-defined aggregates or operators.
>
> (Note that I think we'd have to resist the temptation to fix that
> by letting CREATE AGGREGATE et al accept support functions that
> take anyarray/anycompatiblearray (etc) interchangeably. A lot of
> the security analysis that went into CVE-2020-14350 depended on
> the assumption that utility commands only do exact lookups of
> support functions. If we tried to be lax about this, we'd
> re-introduce the possibility for hostile capture of function
> references in extension scripts.)
>
> Another interesting issue, not seen in the attached but which
> came up while I was experimenting with the more aggressive patch,
> was this failure in the polymorphism test:
>
> select max(histogram_bounds) from pg_stats where tablename = 'pg_am';
> -ERROR: cannot compare arrays of different element types
> +ERROR: function max(anyarray) does not exist
>
> That's because we don't accept pg_stats.histogram_bounds (of
> declared type anyarray) as a valid input for anycompatiblearray.
> I wonder if that isn't a bug we need to fix in the anycompatible
> patch, independently of anything else. We'd not be able to deduce
> an actual element type from such an input, but we already cannot
> do so when we match it to an anyarray parameter.
>
> Anyway, attached find
>
> 0001 - updates Vik's original patch to HEAD and tweaks the
> grammar in the docs a bit.
>
> 0002 - add-on patch to convert array_append, array_prepend,
> array_cat, array_position, array_positions, array_remove,
> array_replace, and width_bucket to use anycompatiblearray.
>
> I think 0001 is committable, but 0002 is just WIP since
> I didn't touch the docs. I'm slightly discouraged about
> whether 0002 is worth proceeding with. Any thoughts?
>

I think so 0002 has sense - more than doc I miss related regress tests, but
it is partially covered by anycompatible tests

Anyway I tested both patches and there is not problem with compilation,
building doc, and make check-world passed

I'll mark this patch as ready for committer

Best regards

Pavel

>
> regards, tom lane
>
>