Message-ID: <48BE9AA0.1070606@esilo.com>
Date: Wed, 03 Sep 2008 10:09:36 -0400
From: Andrew Chernow <ac@esilo.com>
User-Agent: Thunderbird 2.0.0.16 (Windows/20080708)
MIME-Version: 1.0
To: Alvaro Herrera <alvherre@commandprompt.com>
CC: Merlin Moncure <mmoncure@gmail.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: libpq object hooks (libpq events)
References: <19342.1210776271@sss.pgh.pa.us>
	<b42b73150805140921v30d24b75m6c230932c61ffdd2@mail.gmail.com>
	<482B1480.1070705@esilo.com> <482B2AC5.7000503@esilo.com>
	<482ECF62.7010901@esilo.com> <12200.1211242959@sss.pgh.pa.us>
	<48323593.4060302@esilo.com> <13642.1211251078@sss.pgh.pa.us>
	<48323F94.6010201@esilo.com> <48325386.2010506@esilo.com>
	<20080903001920.GT12610@alvh.no-ip.org>
In-Reply-To: <20080903001920.GT12610@alvh.no-ip.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Alvaro Herrera wrote:
> 
> There's one thing that seems a bit baroque, which is the
> PG_COPYRES_USE_ATTRS stuff in PQcopyResult.  I think that flag
> introduces different enough behavior that it should be a routine of its
> own, say PQcopyResultAttrs.  That way you would leave out the two extra
> params in PQcopyResult.
> 
> Oh -- one last thing.  I am not really sure about the flags to
> PQcopyResult.  Should there really be flags to _remove_ behavior,
> instead of flags that add?  i.e. instead of having "0" copy everything,
> and have to pass flags for things not to copy, wouldn't it be cleaner to
> have 0 copy only base stuff, and require flags to copy extra things?
> 
> a "name" is attached to every event proc, so that it can be 
> reported in error messages
> 

Can someone confirm that an event 'name' should be re-introduced, as 
suggested by Alvaro?

Can I get a happy or sad face in regards to below?

New options which add instead of remove.

#define PG_COPYRES_ATTRS          0x01
#define PG_COPYRES_TUPLES         0x02 /* Implies PG_COPYRES_ATTRS */
#define PG_COPYRES_EVENTS         0x04
#define PG_COPYRES_NOTICEHOOKS    0x08

// tuples implies attrs, you need the attrs to copy the tuples.
if(options & PG_COPYRES_TUPLES)
   options |= PG_COPYRES_ATTRS; // auto set option

In regards to copying the attrs, the PQcopyResult still needs the 
ability to copy the source result's attrs.  Although, it doesn't need 
the ability to provide custom attrs (which I removed).  New prototype 
for copyresult:

PGresult *
PQcopyResult(const PGresult *src, int options);

I then added a PQsetResultAttrs.  copyresultattrs didn't seem like the 
correct name because you are no longer copying attrs from a source 
result.  You are providing the attrs to 'set'.

int
PQsetResultAttrs(PGresult *res, int numAttributes,
   PGresAttDesc *attDescs);

If the result provided to setattrs already contains attributes, I have 
the function failing (can't overwrite existing attrs).  I think this is 
good behavior....

When PQcopyResult needs to copy the source result's attrs, it calls 
PQsetResultAttrs.

/* Wants attrs */
if((options & PG_COPYRES_ATTRS) &&
    !PQsetResultAttrs(dest, src->numAttributes, src->attDescs))
{
   PQclear(dest);
   return NULL;
}

So, there is some nice code reuse which indicates to me the code is 
segmented well (copyres & setattrs).

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/