Prev: named parameters in SQL functions
Next: [HACKERS] ORDER BY vs. volatile functions
From: Marko Tiikkaja on 15 Nov 2009 16:27
I wrote:
> Attached is the latest version of this patch.

Here's that same patch in context diff format. Sorry for the noise.


Regards,
Marko Tiikkaja
From: Alex Hunsaker on 17 Nov 2009 10:52
On Tue, Nov 17, 2009 at 03:54, Marko Tiikkaja
<marko.tiikkaja(a)cs.helsinki.fi> wrote:
>> Also, after reading through the previous threads; it was not
>> immediately obvious that you dealt with
>> http://archives.postgresql.org/pgsql-hackers/2009-10/msg00566.php by
>> only allowing selects or values at the top level of with.
>
> This is actually just something missing from the current implementation.
>  The relevant posts are in the same thread:

> http://archives.postgresql.org/pgsql-hackers/2009-10/msg00558.php and
> the two follow-ups.  The comment in ExecutePlan() is a bit misleading.

Hrm I tried the various forms of:
with x as ( ... ) insert/update/delete

and could not get any of them to work. So I assumed the comment
about only SELECT and values were allowed was correct. Maybe a
function that does an insert or update at the top level could get it
to break?

> What I meant is that we don't call GetCurrentCommandId() in
> standard_ExecutorStart().  Instead we get a new CID for every CTE with
> INSERT/UPDATE/DELETE.  That comment tried to point out the fact that
> this strategy could fail if there was a non-SELECT query as the
> top-level statement because we wouldn't increment the CID after the last
> CTE.

Right... Which I thought was more or less the recommendation? Guess
Ill have to go re-read that discussion.

> I did it this way because it works well for the purposes of this
> patch and I didn't see an obvious way to determine whether we need a new
> CID for the top-level statement or not.
>
> I'll send an updated patch in a couple of days.

Peachy.

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 26 Nov 2009 13:24
Marko Tiikkaja <marko.tiikkaja(a)cs.helsinki.fi> writes:
> Attached is the latest version of the patch.

I started to look at this patch and soon noted that a very substantial
fraction of the delta had to do with getting rid of dependencies on
estate->es_result_relation_info. It seemed to me that if we were going
to do that, we should try to get rid of the field altogether, so I
looked at what that would take. Unfortunately, there's a problem with
that, which I think proves the patch's approach invalid as well. With
the patch's changes, the only remaining user of es_result_relation_info
is ExecContextForcesOids(), which is only called during plan node
initialization. The patch supposes that it's sufficient to set up
es_result_relation_info while a ModifyTable node is initializing its
child nodes. What this misses is EvalPlanQual, which can require
initialization of a new plan tree during execution. To make it work
we'd need to be sure to set up es_result_relation_info during execution
as well, which pretty much destroys any gain from the proposed
refactoring.

When I realized this, my first thought was that we might as well drop
all the proposed changes that involve avoiding use of
es_result_relation_info. I was wondering though whether you had a
functional reason for getting rid of them, or if it was just trying to
tidy the code a bit?

I did think of a plan B: we could get rid of ExecContextForcesOids()
altogether and let plan nodes always do whatever seems locally most
efficient about OIDs. The consequence of this would be that if we
are doing INSERT or SELECT INTO and the plan tree produces the wrong
has-OIDs state, we would need to insert a junkfilter to fix it, which
would represent processing that would be unnecessary if there was no
other reason to have a junkfilter (ie, no junk columns in the result).
This actually does not affect UPDATE, which always has a junk TID
column so always needs a junkfilter anyway; nor DELETE, which doesn't
need to produce tuples for insertion. At the time we put in
ExecContextForcesOids() it seemed that there was enough of a use-case
to justify klugery to avoid the extra filtering overhead. However,
since OIDs in user tables have been deprecated for several versions
now, I'm thinking that maybe the case doesn't arise often enough to
justify keeping such a wart in the executor.

Comments?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Marko Tiikkaja on 26 Nov 2009 13:39
Tom Lane wrote:
> What this misses is EvalPlanQual, which can require
> initialization of a new plan tree during execution.

Agh. You're right, I missed that.

> When I realized this, my first thought was that we might as well drop
> all the proposed changes that involve avoiding use of
> es_result_relation_info. I was wondering though whether you had a
> functional reason for getting rid of them, or if it was just trying to
> tidy the code a bit?

The latter.

> However,
> since OIDs in user tables have been deprecated for several versions
> now, I'm thinking that maybe the case doesn't arise often enough to
> justify keeping such a wart in the executor.

Under the circumstances I'd lean towards this option.


Regards,
Marko Tiikkaja


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 27 Nov 2009 12:42
Marko Tiikkaja <marko.tiikkaja(a)cs.helsinki.fi> writes:
> Tom Lane wrote:
>> since OIDs in user tables have been deprecated for several versions
>> now, I'm thinking that maybe the case doesn't arise often enough to
>> justify keeping such a wart in the executor.

> Under the circumstances I'd lean towards this option.

I've been fooling around with this further and have gotten as far as
the attached patch. It passes regression tests but suffers from an
additional performance loss: the physical-tlist optimization is disabled
when scanning a relation having OIDs. (That is, we'll always use
ExecProject even if the scan is "SELECT * FROM ...".) I think this loss
is worth worrying about since it would apply to queries on system
catalogs, even if the database has no OIDs in user tables. The trick
is to make the knowledge of the required hasoid state available at
ExecAssignResultType time, so that the plan node's result tupdesc is
constructed correctly.

What seems like the best bet is to merge ExecAssignResultTypeFromTL
and ExecAssignScanProjectionInfo into a single function that should
be used by scan node types. It'll do the determination of whether
a physical-tlist optimization is possible, and then set up both the
output tupdesc and the projection info accordingly. This will make the
patch diff a good bit longer but not much more interesting, so I'm
sending it along at this stage.

I think this is worth doing since it cleans up one of the grottier
parts of executor initialization. The whole thing around
ExecContextForcesOids was never pretty, and it's been the source of
more than one bug if memory serves.

Comments?

regards, tom lane
 |  Next  |  Last
Pages: 1 2 3
Prev: named parameters in SQL functions
Next: [HACKERS] ORDER BY vs. volatile functions