Re: [HACKERS] Fix bug in handling of dropped columns in pltcl triggers
Jim Nasby 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix bug in handling of dropped columns in pltcl triggers
Jim Nasby 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix bug in handling of dropped columns in pltcl triggers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix bug in handling of dropped columns in pltcl triggers
Michael Paquier writes: > On Tue, Nov 1, 2016 at 4:17 AM, Jim Nasby 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix bug in handling of dropped columns in pltcl triggers
On Tue, Nov 1, 2016 at 4:17 AM, Jim Nasby 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: $TG_relatts A Tcl list of the table column names, prefixed with an empty list element. So looking up a column name in the list with Tcl's lsearch command returns the element's number starting with 1 for the first column, the same way the columns are customarily numbered in PostgreSQL. (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.) As this is a behavior present since 2004, it does not sound wise to change it. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers