From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tatsuo Ishii
> No. Parse, bind and Execute message indivually stops and starts the
> statement_timeout timer with the patch. Unless there's a lock conflict,
> parse and bind will take very short time. So actually users could only
> observe the timeout in execute message though.
Where is the statement_timeout timer stopped when processing Parse and Bind messages? Do you mean the following
sequenceof operations are performed in this patch?
Parse(statement1) start timer stop timer
Bind(statement1, portal1) start timer stop timer
Execute(portal1) start timer stop timer
Sync
It looks like the patch does the following. I think this is desirable, because starting and stopping the timer for
eachmessage may be costly as Tom said.
Parse(statement1) start timer
Bind(statement1, portal1)
Execute(portal1) stop timer
Sync
> > (3)
> > + /*
> > + * Sanity check
> > + */
> > + if (!xact_started)
> > + {
> > + ereport(ERROR,
> > +
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("Transaction
> must have been already started to set statement timeout")));
> > + }
> >
> > I think this fragment can be deleted, because enable/disable_timeout()
> is used only at limited places in postgres.c, so I don't see the chance
> of misuse.
>
> I'd suggest leave it as it is. Because it might be possible that the function
> is used in different place in the future. Or at least we should document
> the pre-condition as a comment.
OK, I favor documenting to reduce code, but I don't have a strong opinion. I'd like to leave this to the committer.
Onething to note is that you can remove { and } in the following code like other places.
+ if (!xact_started)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("Transaction must have been already started to set statement timeout")));
+ }
Regards
Takayuki Tsunakawa
