Fix bug in handling of dropped columns in pltcl triggers

While reviewing code coverage in pltcl, I uncovered a bug in trigger
function return handling. If you returned the munged name of a dropped
column, that would silently be ignored. It would be unusual to hit this,
since dropped columns end up with names like
".......pg.dropped.2.......", but since that's still a legitimate name
for a column silently ignoring it seems rather bogus.

Work sponsored by FlightAware.

https://github.com/postgres/postgres/compare/master...decibel:tcl_dropped
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

On Tue, Nov 1, 2016 at 4:17 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> While reviewing code coverage in pltcl, I uncovered a bug in trigger
> function return handling. If you returned the munged name of a dropped
> column, that would silently be ignored. It would be unusual to hit this,
> since dropped columns end up with names like ".......pg.dropped.2.......",
> but since that's still a legitimate name for a column silently ignoring it
> seems rather bogus.

It seems to me that this patch breaks $TG_relatts and what existing
applications would except from it:
<varlistentry>
<term><varname>$TG_relatts</varname></term>
<listitem>
<para>
A Tcl list of the table column names, prefixed with an empty list
element. So looking up a column name in the list with
<application>Tcl</>'s
<function>lsearch</> command returns the element's number starting
with 1 for the first column, the same way the columns are customarily
numbered in <productname>PostgreSQL</productname>. (Empty list
elements also appear in the positions of columns that have been
dropped, so that the attribute numbering is correct for columns
to their right.)
</para>
</listitem>
</varlistentry>
As this is a behavior present since 2004, it does not sound wise to change it.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Tue, Nov 1, 2016 at 4:17 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> While reviewing code coverage in pltcl, I uncovered a bug in trigger
>> function return handling. If you returned the munged name of a dropped
>> column, that would silently be ignored. It would be unusual to hit this,
>> since dropped columns end up with names like ".......pg.dropped.2.......",
>> but since that's still a legitimate name for a column silently ignoring it
>> seems rather bogus.

> It seems to me that this patch breaks $TG_relatts and what existing
> applications would except from it:

I'm not sure how it would do that. What's being changed here is the
code for processing the returned tuple; it doesn't change what is put
into $TG_relatts. Also, per the manual,

> (Empty list
> elements also appear in the positions of columns that have been
> dropped, so that the attribute numbering is correct for columns
> to their right.)

If the trigger function were blindly using that information without any
exception for dropped columns, then what it would provide here as the
column name is an empty string, not ".......pg.dropped.2.......", so that
it would get a failure anyway. This change wouldn't affect that.

So I'm inclined to agree that we should change this, but I think Jim's
patch doesn't go far enough: what we really ought to do is change
SPI_fnumber itself to reject matches to dropped columns. I just did
a survey of callers, and only a small minority of them are explicitly
testing for a match to a dropped column, but it doesn't look to me
like any of the rest are really prepared to do something reasonable
with one. I find it impossible to think of a situation where a caller
would want to treat a match to a dropped column as valid.

My proposal therefore is for SPI_fnumber to ignore (not match to)
dropped columns, and to remove any caller-side attisdropped tests that
thereby become redundant.

It's possible that it'd make sense for pltcl_trigger_handler to ignore
empty-string column names in the returned list, so that the behavior
with stupid trigger functions would be a bit more forgiving; but that
is more or less independent of this patch.

regards, tom lane

On 11/4/16 4:28 PM, Tom Lane wrote:
> My proposal therefore is for SPI_fnumber to ignore (not match to)
> dropped columns, and to remove any caller-side attisdropped tests that
> thereby become redundant.

Yeah, SPI users certainly shouldn't need to worry about attisdropped,
and exposing attnum doesn't seem like a good idea either.

> It's possible that it'd make sense for pltcl_trigger_handler to ignore
> empty-string column names in the returned list, so that the behavior
> with stupid trigger functions would be a bit more forgiving; but that
> is more or less independent of this patch.

I'm a bit reluctant to do that since it'd be nice to be consistent with
regular pltcl functions returning composites. The same kind of issue
exists with the holes in $TG_relatts; we shouldn't be exposing the
details of attnum that way. Any code that's expecting those holes is
going to blow up after a dump/restore anyway.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> writes:
> On 11/4/16 4:28 PM, Tom Lane wrote:
>> It's possible that it'd make sense for pltcl_trigger_handler to ignore
>> empty-string column names in the returned list, so that the behavior
>> with stupid trigger functions would be a bit more forgiving; but that
>> is more or less independent of this patch.

> I'm a bit reluctant to do that since it'd be nice to be consistent with
> regular pltcl functions returning composites. The same kind of issue
> exists with the holes in $TG_relatts; we shouldn't be exposing the
> details of attnum that way. Any code that's expecting those holes is
> going to blow up after a dump/restore anyway.

Hm. Offhand it seems like the functions that pltcl itself exposes don't
really do anything that would depend on $TG_relatts indexes matching
physical column numbers. The only way you could write a pltcl function
that would depend on that would be to have it do some catalog queries that
expect the indexes to match pg_attribute.attnum. That's possible I guess
but it seems neither likely nor good practice.

I think I'd be in favor of trying to remove the business about having
empty-string entries in $TG_relatts. Do you want to draft a patch
for that?

regards, tom lane

Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com> writes:
> On 11/4/16 4:28 PM, Tom Lane wrote:
>> My proposal therefore is for SPI_fnumber to ignore (not match to)
>> dropped columns, and to remove any caller-side attisdropped tests that
>> thereby become redundant.

> Yeah, SPI users certainly shouldn't need to worry about attisdropped,
> and exposing attnum doesn't seem like a good idea either.

OK, I pushed a patch to centralize this in SPI_fnumber.

regards, tom lane