Обсуждение: Re: [GENERAL] How to extract a value from a record using attnum or attname?

[moving to -hackers with BC to -general]

Dimitri Fontaine <dimitri@2ndQuadrant.fr> wrote:
> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
>
>> PL/pgSQL seems tantalizingly close to being useful for developing
>> a generalized trigger function for notifying the client of
>> changes. I don't know whether I'm missing something or whether
>> we're missing a potentially useful feature here.  Does anyone see
>> how to fill in where the commented question is, or do I need to
>> write this function in C?
>
> See those:
>
> http://tapoueh.org/articles/blog/_Dynamic_Triggers_in_PLpgSQL.html
>
http://www.pgsql.cz/index.php/PL_toolbox_%28en%29#Record.27s_functions
>
>>   for i in array_lower(keycols, 1)..array_upper(keycols, 1) loop
>>     select quote_ident(attname) from pg_catalog.pg_attribute
>>       where attrelid = tg_relid and attnum = keycols[i]::oid
>
> Beware of attisdropped, which I've not fixed in the published URL
> before (the tapoueh.org one).

Thanks.

In the absence of an earlier response, though, I went ahead and
wrote the attached, which has passed some initial programmer testing
and is scheduled to start business analyst testing tomorrow with the
application software for production deployment in a couple months.
We probably won't go back to PL/pgSQL for this now.

I'm assuming that while I have an AccessShareLock on the index
relation for the primary key, any attributes it tells me are used by
that relation will not have the attisdropped flag set?

What this trigger function does is to issue a NOTIFY to the channel
specified as a parameter to the function in CREATE TRIGGER (with
'tcn' as the default), and a payload consisting of the table name, a
code for the operation (Insert, Update, or Delete), and the primary
key values.  So, an update to a Party record for us might generate
this NOTIFY payload:

"Party",U,"countyNo"='71',"caseNo"='2011CF001234',"partyNo"='1'

This is one of those things which our shop needs, but I was planning
to post it for the first 9.2 CF fest to see if anyone else was
interested.  It struck me while typing this post that for general
use the schema would probably need to be in there, but I'll worry
about that later, if anyone else *is* interested.  If anyone wants
it I can provide Java code to tear apart the NOTIFY payloads using
the Pattern and Matches classes.

I'll add to the first 9.2 CF referencing this post.

-Kevin

On 02/22/2011 05:32 PM, Kevin Grittner wrote:
> [moving to -hackers with BC to -general]
>
> Dimitri Fontaine<dimitri@2ndQuadrant.fr>  wrote:
>> "Kevin Grittner"<Kevin.Grittner@wicourts.gov>  writes:
>>
>>> PL/pgSQL seems tantalizingly close to being useful for developing
>>> a generalized trigger function for notifying the client of
>>> changes. I don't know whether I'm missing something or whether
>>> we're missing a potentially useful feature here.  Does anyone see
>>> how to fill in where the commented question is, or do I need to
>>> write this function in C?
>> See those:
>>
>> http://tapoueh.org/articles/blog/_Dynamic_Triggers_in_PLpgSQL.html
>>
> http://www.pgsql.cz/index.php/PL_toolbox_%28en%29#Record.27s_functions
>>>    for i in array_lower(keycols, 1)..array_upper(keycols, 1) loop
>>>      select quote_ident(attname) from pg_catalog.pg_attribute
>>>        where attrelid = tg_relid and attnum = keycols[i]::oid
>> Beware of attisdropped, which I've not fixed in the published URL
>> before (the tapoueh.org one).
>
> Thanks.
>
> In the absence of an earlier response, though, I went ahead and
> wrote the attached, which has passed some initial programmer testing
> and is scheduled to start business analyst testing tomorrow with the
> application software for production deployment in a couple months.
> We probably won't go back to PL/pgSQL for this now.
>
> I'm assuming that while I have an AccessShareLock on the index
> relation for the primary key, any attributes it tells me are used by
> that relation will not have the attisdropped flag set?
>
> What this trigger function does is to issue a NOTIFY to the channel
> specified as a parameter to the function in CREATE TRIGGER (with
> 'tcn' as the default), and a payload consisting of the table name, a
> code for the operation (Insert, Update, or Delete), and the primary
> key values.  So, an update to a Party record for us might generate
> this NOTIFY payload:
>
> "Party",U,"countyNo"='71',"caseNo"='2011CF001234',"partyNo"='1'
>
> This is one of those things which our shop needs, but I was planning
> to post it for the first 9.2 CF fest to see if anyone else was
> interested.  It struck me while typing this post that for general
> use the schema would probably need to be in there, but I'll worry
> about that later, if anyone else *is* interested.  If anyone wants
> it I can provide Java code to tear apart the NOTIFY payloads using
> the Pattern and Matches classes.
>
> I'll add to the first 9.2 CF referencing this post.
>


Have you performance tested it? Scanning pg_index for index columns for 
each row strikes me as likely to be unpleasant.

Also, the error messages seem to need a bit of work (no wonder they 
seemed familiar to me :) )

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> wrote:
> Have you performance tested it? Scanning pg_index for index
> columns for each row strikes me as likely to be unpleasant.
I haven't, yet.  I had rather assumed that the index info for a
relation would have a high probability of being cached during
execution of an AFTER trigger for that relation, so I think we're
talking RAM access here.  It didn't seem sane to try to create an
HTAB for this and worry about invalidation of it, etc.  If there's a
faster way to get to the info without going to such extremes, I'd be
happy to hear them.  (At least I avoided building and parsing a
query to get at it.)
> Also, the error messages seem to need a bit of work (no wonder
> they seemed familiar to me :) )
[blush]  I was just trying to write code with "fits in with
surrounding code", as recommended.  You mean I should change the
function name in the message from the name of the function I copied
*from* to the name of the function I copied *to*?  Well, I *guess*
it still fits in....  ;-)
Seriously, thanks for pointing that out.  Will fix.
-Kevin

Excerpts from Kevin Grittner's message of mar feb 22 20:29:26 -0300 2011:
> Andrew Dunstan <andrew@dunslane.net> wrote:
>  
> > Have you performance tested it? Scanning pg_index for index
> > columns for each row strikes me as likely to be unpleasant.
>  
> I haven't, yet.  I had rather assumed that the index info for a
> relation would have a high probability of being cached during
> execution of an AFTER trigger for that relation, so I think we're
> talking RAM access here.  It didn't seem sane to try to create an
> HTAB for this and worry about invalidation of it, etc.  If there's a
> faster way to get to the info without going to such extremes, I'd be
> happy to hear them.  (At least I avoided building and parsing a
> query to get at it.)

I think it'd be better to use RelationGetIndexList (which gets the index
list from relcache) and fetch the index tuples from syscache; see
relationHasPrimaryKey for sample code.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> wrote:

> I think it'd be better to use RelationGetIndexList (which gets the
> index list from relcache) and fetch the index tuples from
> syscache; see relationHasPrimaryKey for sample code.

Thanks.  Patch done that way attached.  Will get it into tomorrow's
system testing here.

-Kevin

Excerpts from Kevin Grittner's message of mié feb 23 13:43:19 -0300 2011:
> Alvaro Herrera <alvherre@commandprompt.com> wrote:
>  
> > I think it'd be better to use RelationGetIndexList (which gets the
> > index list from relcache) and fetch the index tuples from
> > syscache; see relationHasPrimaryKey for sample code.
>  
> Thanks.  Patch done that way attached.  Will get it into tomorrow's
> system testing here.

Why not use quote_identifier and quote_literal_cstr instead of this new
strcpy thing?  Also, you don't really need spi.h do you?

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> wrote:
> Why not use quote_identifier and quote_literal_cstr instead of
> this new strcpy thing?
We've got various types of software that will be parsing these
payloads, and it's a little easier to parse if the quoting is
unconditional.  If that's a barrier to acceptance we could use
the functions which quote conditionally and adjust our regular
expressions.
Probably one reason we had a bias toward quoting is that every
single application table name we use has at least on uppercase
letter and about 95% of our column names do.
> Also, you don't really need spi.h do you?
It's using these functions:
SPI_getrelname
SPI_fname
SPI_getvalue
If there's a better way to get the info, I'm game. 
-Kevin

Excerpts from Kevin Grittner's message of mié feb 23 16:20:16 -0300 2011:
> Alvaro Herrera <alvherre@commandprompt.com> wrote:
>  
> > Why not use quote_identifier and quote_literal_cstr instead of
> > this new strcpy thing?
>  
> We've got various types of software that will be parsing these
> payloads, and it's a little easier to parse if the quoting is
> unconditional.  If that's a barrier to acceptance we could use
> the functions which quote conditionally and adjust our regular
> expressions.

No strong opinion on this, really, but your strcpy should use a
StringInfo buffer instead of the char[200].  That's going to bite
someone.

> Probably one reason we had a bias toward quoting is that every
> single application table name we use has at least on uppercase
> letter and about 95% of our column names do.

Makes sense.

> > Also, you don't really need spi.h do you?
>  
> It's using these functions:
>  
> SPI_getrelname
> SPI_fname
> SPI_getvalue
>  
> If there's a better way to get the info, I'm game. 

I think you could get away without the first two (in particular get rid
of the memleak with SPI_getrelname), but the last one would require
something more involved.  No strong opinion, I just failed to see those
calls in there.

Is this intended for 9.1?

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Feb 23, 2011 at 2:48 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Is this intended for 9.1?

Kevin already expressed his intention to add this to the first 9.2CF.
It's far too late to BEGIN discussing new features for 9.1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of mié feb 23 17:03:23 -0300 2011:
> On Wed, Feb 23, 2011 at 2:48 PM, Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
> > Is this intended for 9.1?
> 
> Kevin already expressed his intention to add this to the first 9.2CF.
> It's far too late to BEGIN discussing new features for 9.1.

Yeah, I see that now.  I'll go review some other patch then.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> wrote:
> Excerpts from Kevin Grittner's message:
> No strong opinion on this, really, but your strcpy should use a
> StringInfo buffer instead of the char[200].  That's going to bite
> someone.
Yeah, this was thrown together in a bit of a hurry because of
development deadlines here, so I cut a few corners based on
knowledge of our particular implementation details.  I know that's
something to change for general acceptance.
>> It's using these functions:
>>  
>> SPI_getrelname
>> SPI_fname
>> SPI_getvalue
>>  
>> If there's a better way to get the info, I'm game. 
> 
> I think you could get away without the first two (in particular
> get rid of the memleak with SPI_getrelname), but the last one
> would require something more involved.  No strong opinion, I just
> failed to see those calls in there.
I thought the trigger would be running in a context which would make
that leak immaterial.  I thought the general advice in such cases is
to *not* do retail freeing of space, but to let it get cleaned up
through release of the memory context.  I'll take another look at
memory contexts around triggers.
> Is this intended for 9.1?
Definitely not.  I added it to the first 9.2 CF, as mentioned in
earlier posts. I was going to hold off posting until the beta was
wrapped, but it seemed reasonable to post the patch in response to
Dimitri's post.  I wasn't intending to suggest it was ready for
general usage; I was mainly just putting it out there to see if
anyone was interested enough in it that I should polish it up for a
proper submission.  For instance, there are no docs or regression
tests yet.
Anyway, I certainly appreciate the pointers, because we have to push
something out to production along these lines in a couple months to
stay on track with the organization's Annual Plan, which we need to
provide to the legislature and are judged against when they
authorize funding each year.  I think your advice will bring this
feature from "it works" to "it works really well".  :-)
-Kevin