Re: [PATCHES] libpq events patch (with sgml docs)

- Added sgml docs.
- Removed the PQpassThroughData and PQresultPassThroughData functions.
- Added an argument to PGEventProc, the passThrough pointer.
- fixed some errors in the code comments due to code shuffles.

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

Andrew Chernow escribió:

> ! printfPQExpBuffer(&conn->errorMessage,
> ! libpq_gettext("PGEventProc \"%s\" failed during PGEVT_CONNRESET event\n"),
> ! conn->events[i].name);
> ! else
> ! printfPQExpBuffer(&conn->errorMessage,
> ! libpq_gettext("PGEventProc \"addr:%p\" failed during PGEVT_CONNRESET event\n"),
> ! conn->events[i].proc);
> ! break;

Please don't do this. It creates extra unnecessary work for
translators. Better create a local var, assign either "name" or
"addr:<value>" to it, and then use that in the message.

(For the record, I'd prefer that the name is made mandatory.)

Oh, BTW: don't post to pgsql-patches. It's deprecated. Use
pgsql-hackers instead.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Andrew Chernow escribió:
>
>> ! printfPQExpBuffer(&conn->errorMessage,
>> ! libpq_gettext("PGEventProc \"%s\" failed during PGEVT_CONNRESET event\n"),
>> ! conn->events[i].name);
>> ! else
>> ! printfPQExpBuffer(&conn->errorMessage,
>> ! libpq_gettext("PGEventProc \"addr:%p\" failed during PGEVT_CONNRESET event\n"),
>> ! conn->events[i].proc);
>> ! break;
>
> Please don't do this. It creates extra unnecessary work for
> translators. Better create a local var, assign either "name" or
> "addr:<value>" to it, and then use that in the message.
>
> (For the record, I'd prefer that the name is made mandatory.)
>

The name is now mandatory, no need to toggle the format string anymore.

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

Andrew Chernow escribió:

> /*
> * PQmakeEmptyPGresult
> * returns a newly allocated, initialized PGresult with given status.
> * If conn is not NULL and status indicates an error, the conn's
> * errorMessage is copied.
> *
> * Note this is exported --- you wouldn't think an application would need
> * to build its own PGresults, but this has proven useful in both libpgtcl
> * and the Perl5 interface, so maybe it's not so unreasonable.
> + *
> + * Updated April 2008 - If conn is not NULL, event states will be copied
> + * from the PGconn to the created PGresult.
> */

Don't do this either. We don't really need to know when the thing was
changed; the comment should just state what the function does. I had
folded the last paragraph into the introductory one, but I think you
lost that part of my change.

> + /* resultalloc the attribute names. The above memcpy has the attr
> + * names pointing at the callers provided attDescs memory.
> + */

"resultalloc"? Why not just "allocate"?

> * PQclear -
> * free's the memory associated with a PGresult
> */

I'd add a comment here stating why the event name is not deallocated;
otherwise it just looks like it's being leaked.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Andrew Chernow escribió:
>
>> * and the Perl5 interface, so maybe it's not so unreasonable.
>> + *
>> + * Updated April 2008 - If conn is not NULL, event states will be copied
>> + * from the PGconn to the created PGresult.
>> */
>
> Don't do this either. We don't really need to know when the thing was
>

Changed that and made it one paragraph.

>> + /* resultalloc the attribute names. The above memcpy has the attr
>> + * names pointing at the callers provided attDescs memory.
>> + */
>
> "resultalloc"? Why not just "allocate"?
>

Because resultalloc is what a PGresult uses to allocate its attribute
names (See line 513 of fe-protocol3.c). Also, PQclear frees its memory
blocks, not individual items like attname.

>> * PQclear -
>> * free's the memory associated with a PGresult
>> */
>
> I'd add a comment here stating why the event name is not deallocated;
> otherwise it just looks like it's being leaked.
>
>

The event name is being deallocated.

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

Andrew Chernow escribió:
> Alvaro Herrera wrote:
>> Andrew Chernow escribió:

>>> * PQclear -
>>> * free's the memory associated with a PGresult
>>> */
>>
>> I'd add a comment here stating why the event name is not deallocated;
>> otherwise it just looks like it's being leaked.
>
> The event name is being deallocated.

Doh! Sorry, you're right. In that case, you're missing a NULL result
check from strdup() in dupEvents() ;-)

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
>
> Doh! Sorry, you're right. In that case, you're missing a NULL result
> check from strdup() in dupEvents() ;-)
>

Missed that one. Good catch :) Update attached.

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

Alvaro Herrera wrote:
>Oh, BTW: don't post to pgsql-patches. It's deprecated. Use
> pgsql-hackers instead.

I thought this too until I read
http://wiki.postgresql.org/wiki/Submitting_a_Patch

Which is correct?

David.

David Rowley escribió:
> Alvaro Herrera wrote:
> >Oh, BTW: don't post to pgsql-patches. It's deprecated. Use
> > pgsql-hackers instead.
>
> I thought this too until I read
> http://wiki.postgresql.org/wiki/Submitting_a_Patch
>
> Which is correct?

I just updated that page and the developer's FAQ to meet the current
reality.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Andrew Chernow <ac(at)esilo(dot)com> writes:
> Missed that one. Good catch :) Update attached.

Looking at this now. Question: why does PQgetResult invoke the
RESULTCREATE event only for non-error results? This seems a rather
odd asymmetry, particularly in view of the fact that a RESULTDESTROY
event will occur for error results. And surely we do not need to
micro-optimize error cases for speed.

regards, tom lane

Tom Lane wrote:
> Andrew Chernow <ac(at)esilo(dot)com> writes:
>> Missed that one. Good catch :) Update attached.
>
> Looking at this now. Question: why does PQgetResult invoke the
> RESULTCREATE event only for non-error results?

It didn't seem useful to have the eventproc implementation allocate its private
storage (or do whatever prep work it does), just to have it PQclear'd once the
user gets the dead result back. I guess we figured an error result typically
has a short life-span, not very useful outside of indicating an error.

Is there a use case you think requires the opposite behavior?

> odd asymmetry, particularly in view of the fact that a RESULTDESTROY
> event will occur for error results. And surely we do not need to
> micro-optimize error cases for speed.
>

It is odd. Actually, it looks like a bug to me. PQgetResult is the behavior we
were after, don't trigger the event if the creation failed. Same thing occurs
during a conn reset. Looks like PQclear needs to check resultStatus before it
triggers RESULTDESTROY events.

But before I fix this and put a patch up, please let me know if you prefer the
opposite behavior ... trigger events on success or failure (including connreset).

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
> Tom Lane wrote:
>> Looking at this now. Question: why does PQgetResult invoke the
>> RESULTCREATE event only for non-error results?

> It didn't seem useful to have the eventproc implementation allocate its private
> storage (or do whatever prep work it does), just to have it PQclear'd once the
> user gets the dead result back. I guess we figured an error result typically
> has a short life-span, not very useful outside of indicating an error.

Well, you could do a PQresultStatus and skip any prep work if you
actually saw a need to micro-optimize such cases.

> It is odd. Actually, it looks like a bug to me. PQgetResult is the
> behavior we were after, don't trigger the event if the creation
> failed. Same thing occurs during a conn reset. Looks like PQclear
> needs to check resultStatus before it triggers RESULTDESTROY events.

If you did that, then you WOULD have a bug. Consider case where you
successfully init two events, and the third one fails. You'll now
change the result's status to ERROR. If you don't RESULTDESTROY
then the first two events will leak whatever storage they allocated.

The reason I suggested not being conditional about the init call was
to reduce the probability of bugs of similar ilks associated with
an event proc assuming that it could only get a DESTROY call for
something it had seen CREATEd --- for example, if it were frobbing
a reference count for some long-lived data, it could very easily
screw up the reference count that way. I suppose that the risk of
an earlier event proc failing means it has to cope with that case
anyway, but I don't think it's a particularly good idea to have
arbitrary asymmetries increasing the odds of a problem.

regards, tom lane

Tom Lane wrote:
>> It is odd. Actually, it looks like a bug to me. PQgetResult is the
>> behavior we were after, don't trigger the event if the creation
>> failed. Same thing occurs during a conn reset. Looks like PQclear
>> needs to check resultStatus before it triggers RESULTDESTROY events.
>
> If you did that, then you WOULD have a bug. Consider case where you
> successfully init two events, and the third one fails. You'll now
> change the result's status to ERROR. If you don't RESULTDESTROY
> then the first two events will leak whatever storage they allocated.
>
> The reason I suggested not being conditional about the init call was
> to reduce the probability of bugs of similar ilks associated with
> an event proc assuming that it could only get a DESTROY call for
> something it had seen CREATEd --- for example, if it were frobbing
> a reference count for some long-lived data, it could very easily
> screw up the reference count that way. I suppose that the risk of
> an earlier event proc failing means it has to cope with that case
> anyway, but I don't think it's a particularly good idea to have
> arbitrary asymmetries increasing the odds of a problem.
>
> regards, tom lane
>
>

Yeah. Good point. I fixed it to always fire events.

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
> Yeah. Good point. I fixed it to always fire events.

Applied with editorial revisions --- I don't think I changed any
functionality, but I fixed a number of corner-case bugs and
editorialized on style a bit.

The general question of symmetry between RESULTCREATE and RESULTDESTROY
callbacks is still bothering me. As committed, PQmakeEmptyPGresult
will copy events into its result, but not fire RESULTCREATE for them
... but they'll get RESULTDESTROY when it's deleted. Is that what we
want? I don't have a lot of faith that PQgetResult is the only place
inside libpq that needs to fire RESULTCREATE, either.

Now, it's questionable how tense we need to be about that as long as
event proc failure aborts calling of later event procs. That means
that procs have to be robust against getting DESTROY with no CREATE
calls in any case. Should we try to make that less uncertain?

regards, tom lane

> Now, it's questionable how tense we need to be about that as long as
> event proc failure aborts calling of later event procs. That means
> that procs have to be robust against getting DESTROY with no CREATE
> calls in any case. Should we try to make that less uncertain?
>
>

I attached a patch that adds a 'needDestroy' member to PGEvent It is set when
resultcreate or resultcopy succeeds and checked during a PQclear. That *should*
resolve the issue of "no resultcreate but gets a resultdestroy".

> The general question of symmetry between RESULTCREATE and RESULTDESTROY
> callbacks is still bothering me. As committed, PQmakeEmptyPGresult
> will copy events into its result, but not fire RESULTCREATE for them
> ... but they'll get RESULTDESTROY when it's deleted. Is that what we
> want?

PQmakeEmptyPGResult was given thought. The problem is every internal function
that generates a result calls PQmakeEmptyPGResult, but those cases should not
fire a resultcreate. resultcreate should be called when the result is fully
constructed (tuples and all) so the eventproc gets a useful PGresult.

One solution is to do something like the below:

PGresult *
PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
{
return pqMakeEmptyPGresult(conn, status, TRUE);
}

PGresult *
pqMakeEmptyPGresult(PGconn *conn, ExecStatusType status, int fireEvents)
{
// existing function, only change is handling fireEvents
}

I am willing to create a patch for this. Is this an acceptable idea?

> I don't have a lot of faith that PQgetResult is the only place
> inside libpq that needs to fire RESULTCREATE, either.
>

I looked again and I didn't see anything. Is there something your thinking of?
ISTM that PQgetResult is called every where a result is created (outside of
PQmakeEmptyPGresult).

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

Andrew Chernow wrote:
> > Now, it's questionable how tense we need to be about that as long as
> > event proc failure aborts calling of later event procs. That means
> > that procs have to be robust against getting DESTROY with no CREATE
> > calls in any case. Should we try to make that less uncertain?
> >
> >
>
> I attached a patch that adds a 'needDestroy' member to PGEvent It is
> set when resultcreate or resultcopy succeeds and checked during a
> PQclear. That *should* resolve the issue of "no resultcreate but gets a
> resultdestroy".
>
>

Shoot. I have a booboo in that last patch. Attached is the corrected version.

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
>>> Now, it's questionable how tense we need to be about that as long as
>>> event proc failure aborts calling of later event procs. That means
>>> that procs have to be robust against getting DESTROY with no CREATE
>>> calls in any case. Should we try to make that less uncertain?

> I attached a patch that adds a 'needDestroy' member to PGEvent It is
> set when resultcreate or resultcopy succeeds and checked during a
> PQclear. That *should* resolve the issue of "no resultcreate but gets
> a resultdestroy".

Some thought would need to be given to how that interacts with
RESULTCOPY. Presumably on the destination side, RESULTCOPY is the
equivalent of RESULTCREATE, ie, don't DESTROY if you didn't get COPY.
But what of the source side? Arguably you shouldn't invoke COPY on
events that were never initialized in this object.

I also think that a little bit of thought should go into whether or
not to call DESTROY on an event that *did* get a CREATE and failed it.
You could argue that one either way: should a failing CREATE operation
be expected to fully clean up after itself, or should it be expected
to leave things in a state where DESTROY can clean up properly?
I'm not entirely sure, but option A might force duplication of code
between CREATE's failure path and DESTROY. Whichever semantics we
choose needs to be documented.

Also, if we choose option B, then the same would hold for REGISTER
versus CONNDESTROY: failing REGISTER should still mean that you get
a CONNDESTROY. So maybe that's an argument for option A, because
that's how REGISTER works now.

Lastly, the idea that was in the back of my mind was to resolve this
issue by unconditionally calling all the event procs at CREATE time,
not by cutting off the later ones if an earlier one failed. Again,
I do not see a performance argument for skipping the extra steps,
and it seems to me that it might improve symmetry/robustness.
I'm not necessarily wedded to that approach but it bears thinking
about.

regards, tom lane

Tom Lane wrote:
>
> Some thought would need to be given to how that interacts with
> RESULTCOPY. Presumably on the destination side, RESULTCOPY is the
> equivalent of RESULTCREATE, ie, don't DESTROY if you didn't get COPY.
> But what of the source side? Arguably you shouldn't invoke COPY on
> events that were never initialized in this object.
>

You are correct. The resultcopy function needs to check if the src
result eventproc was initialized. BUT, that should not be possible
unless someone is not checking return values. Meaning, the only way to
have an uninitialized eventproc in this instance is if something failed
during a resultcreate. Actually, if you call PQmakeEmptyPGResult then
you can also run into this problem. That can be solved by adding an
argument to makeResult as I previously suggested.

> I also think that a little bit of thought should go into whether or
> not to call DESTROY on an event that *did* get a CREATE and failed it.
> You could argue that one either way: should a failing CREATE operation
> be expected to fully clean up after itself, or should it be expected
> to leave things in a state where DESTROY can clean up properly?
> I'm not entirely sure, but option A might force duplication of code
> between CREATE's failure path and DESTROY. Whichever semantics we
> choose needs to be documented.
>

If a resultcreate fails, than I think that resultdestroy should not be
delivered to that eventproc (same for resultcopy). That is how register
works and how I saw this patch working (eventhough it appears I have a
few logical errors). I don't think it makes sense to do it otherwise,
it would be like calling free after a malloc failure.

The needDestroy member should be called resultInitialized. Although the
conn doesn't reference the 'resultInitialized' member, I should
initialize it to FALSE. I did not do this in the last patch ...
register function.

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
> Tom Lane wrote:
>> Some thought would need to be given to how that interacts with
>> RESULTCOPY. Presumably on the destination side, RESULTCOPY is the
>> equivalent of RESULTCREATE, ie, don't DESTROY if you didn't get COPY.
>> But what of the source side? Arguably you shouldn't invoke COPY on
>> events that were never initialized in this object.

> You are correct. The resultcopy function needs to check if the src
> result eventproc was initialized. BUT, that should not be possible
> unless someone is not checking return values. Meaning, the only way to
> have an uninitialized eventproc in this instance is if something failed
> during a resultcreate. Actually, if you call PQmakeEmptyPGResult then
> you can also run into this problem. That can be solved by adding an
> argument to makeResult as I previously suggested.

I still think it would be a good idea to expend a couple more lines in
PQcopyResult to cover the case --- the likelihood of there being code
paths that make an empty result and never invoke RESULTCREATE just seems
too high. Defensive programming 'n all that. In any case I'm not
convinced that we should force a RESULTCREATE cycle on external callers
of PQmakeEmptyPGResult. If you guys didn't see a need for it in your
development then it probably doesn't exist.

> If a resultcreate fails, than I think that resultdestroy should not be
> delivered to that eventproc (same for resultcopy). That is how register
> works and how I saw this patch working (eventhough it appears I have a
> few logical errors). I don't think it makes sense to do it otherwise,
> it would be like calling free after a malloc failure.

I can live with that definition, but please document it.

> The needDestroy member should be called resultInitialized.

Yeah, probably, so that you can set it FALSE in conn events and continue
to use memcpy to move things over.

regards, tom lane

New patch following our discussion with updated docs.

>> few logical errors). I don't think it makes sense to do it otherwise,
>> it would be like calling free after a malloc failure.
>
> I can live with that definition, but please document it.
>
>

To build on this analogy, PGEVT_CONNRESET is like a realloc. Meaning,
the initial malloc "PGEVT_REGISTER" worked by the realloc
"PGEVT_CONNRESET" didn't ... you still have free "PGEVT_CONNDESTROY" the
initial. Its documented that way. Basically if a register succeeds, a
destroy will always be sent regardless of what happens with a reset.

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

Andrew Chernow wrote:
> New patch following our discussion with updated docs.
>
>>> few logical errors). I don't think it makes sense to do it
>>> otherwise, it would be like calling free after a malloc failure.
>>
>> I can live with that definition, but please document it.
>>
>>
>
> To build on this analogy, PGEVT_CONNRESET is like a realloc. Meaning,
> the initial malloc "PGEVT_REGISTER" worked by the realloc
> "PGEVT_CONNRESET" didn't ... you still have free "PGEVT_CONNDESTROY" the
> initial. Its documented that way. Basically if a register succeeds, a
> destroy will always be sent regardless of what happens with a reset.
>
>

I attached the wrong patch. I'm sorry.

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

Are there any plans to commit these libpq-events changes this fest?

http://archives.postgresql.org/pgsql-hackers/2008-09/msg01109.php

I wanted to release an updated libpqtypes but am waiting on the above
patch. If not, I'll release it now.

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
> Are there any plans to commit these libpq-events changes this fest?

Sorry about that, I got distracted. Will look at it shortly.

regards, tom lane

Andrew Chernow <ac(at)esilo(dot)com> writes:
>> To build on this analogy, PGEVT_CONNRESET is like a realloc. Meaning,
>> the initial malloc "PGEVT_REGISTER" worked by the realloc
>> "PGEVT_CONNRESET" didn't ... you still have free "PGEVT_CONNDESTROY" the
>> initial. Its documented that way. Basically if a register succeeds, a
>> destroy will always be sent regardless of what happens with a reset.

> I attached the wrong patch. I'm sorry.

I had a further thought about this: after applying this patch, it is
essentially useless for the exposed PQmakeEmptyPGresult function to
copy events into the result. If it doesn't give them a RESULTCREATE
call, then they cannot receive RESULTCOPY or RESULTDESTROY either,
so they might as well not be there.

The argument for not having PQmakeEmptyPGresult fire RESULTCREATE still
makes sense, but I am thinking that maybe what we ought to do is expose
a new function named something like PQfireResultCreateEvents() that just
does that. This would allow an application to exactly emulate what
PQgetResult does: make an empty PGresult, fill it, then fire the create
events.

I'll go ahead and apply this patch in a little bit, but if you concur
with the above reasoning, please put together a followon patch to add
such a function.

regards, tom lane

Tom Lane wrote:
>
> I'll go ahead and apply this patch in a little bit, but if you concur
> with the above reasoning, please put together a followon patch to add
> such a function.
>
> regards, tom lane
>
>

I attached a patch. You have to copy the events in PQmakeEmptyPGResult
because there is no where else to do this, other than copyresult but
that is different because it copies from a result not a conn.

PQmakeEmptyPGResult - must copy events here
PQsetResultAttrs - set attributes
PQsetvalue - set tuple values
PQfireResultCreateEvents(conn,res) - now fire resultcreate event

PQgetResult now calls PQfireResultCreateEvents.

BTW, the event system might be an alternative solution for PGNoticeHooks
(PGEVT_NOTICE).

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

On Fri, Sep 19, 2008 at 2:14 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>
> BTW, the event system might be an alternative solution for PGNoticeHooks
> (PGEVT_NOTICE).
>

Another possible use of the event hooks -- just spitballing here -- is
to generate an event when a notification comes through (you would
still receive events the old way., by command or PQconsumeInput).
Maybe this would eventually replace the current notification interface
(wasn't this going to be changed anyways?)

merlin

BTW, why are all the conn parameters to events declared "const"? Isn't
the reason for passing them in mainly to give the event proc a chance
to issue queries?

regards, tom lane

Tom Lane wrote:
> BTW, why are all the conn parameters to events declared "const"? Isn't

Because it looked prettier? Kidding. No idea, do you want me to change
that or do you want to?

> the reason for passing them in mainly to give the event proc a chance
> to issue queries?
>
>

Partly. You also want to give the eventproc a chance to issue a
PQinstanceData call, so it can copy stuff to the created result.

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
> Tom Lane wrote:
>> BTW, why are all the conn parameters to events declared "const"? Isn't

> Because it looked prettier? Kidding. No idea, do you want me to change
> that or do you want to?

I'll fix it; it's hardly worth passing a patch around for.

regards, tom lane

Andrew Chernow <ac(at)esilo(dot)com> writes:
> I attached a patch. You have to copy the events in PQmakeEmptyPGResult
> because there is no where else to do this, other than copyresult but
> that is different because it copies from a result not a conn.

Applied ...

> PQgetResult now calls PQfireResultCreateEvents.

... except I didn't do that because the error handling didn't seem
appropriate. Since PQmakeEmptyPGResult allows a null conn,
PQfireResultCreateEvents ought to as well. So I just made it return
false on failure.

regards, tom lane

"Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
> On Fri, Sep 19, 2008 at 2:14 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> BTW, the event system might be an alternative solution for PGNoticeHooks
>> (PGEVT_NOTICE).

> Another possible use of the event hooks -- just spitballing here -- is
> to generate an event when a notification comes through (you would
> still receive events the old way., by command or PQconsumeInput).

Seems rather pointless; you'd just be adding multiple ways to do things
we can do perfectly well now.

> Maybe this would eventually replace the current notification interface
> (wasn't this going to be changed anyways?)

No, I don't think anyone was unhappy with the libpq API for it.

regards, tom lane