Horton

Thread

Commits

  1. Avoid O(N^2) cost in ExecFindRowMark().

  2. Remove some unnecessary fields from Plan trees.

  3. Restore sane locking behavior during parallel query.

  4. Remove more redundant relation locking during executor startup.

  5. In the executor, use an array of pointers to access the rangetable.

  6. Centralize executor's opening/closing of Relations for rangetable entries.

  7. Change executor to just Assert that table locks were already obtained.

  8. Change rewriter/planner/executor/plancache to depend on RTE rellockmode.

  9. Add assertions that we hold some relevant lock during relation open.

  10. Create an RTE field to record the query's lock mode for each relation.

  1. executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-08-16T08:22:51Z

    This is the continuation of the discussion at:

https://www.postgresql.org/message-id/7500.1531920772%40sss.pgh.pa.us

Actually, for more background on what I've written below, reading this
email in the same discussion would help:

https://www.postgresql.org/message-id/4114.1531674142@sss.pgh.pa.us


Attached find a series of patches, the first of which tries to implement
the main topic discussed in the above email, which is to eliminate
execution-time locking of relations in PlannedStmt.rtable.  One of the
other patches removes the plan node fields that are obsoleted by
eliminating execution-time locking of relations.  Those fields served no
purpose beside telling the executor which relations to lock, or more
precisely which relations to lock before initializing the plan tree so
that we don't end up upgrading lock strength due to same relation being
both a source relation and target relation.

When working on that, I noticed that planner fails to remove PlanRowMarks
of relations that won't be scanned by a given plan, which results in
executor redundantly locking relations that the planner already deemed
unnecessary to scan.  The locking would be gone with one of the proposed
patches, but there are still a couple of overheads during executor
initialization of having all those PlanRowMarks.  For example,
ExecInitLockRows or ExecInitModifyTable calling ExecFindRowMark would end
up going over PlanRowMarks that won't ever be used, which especially grows
worse with many partitions.

Short description of each patch follows:

0001-Don-t-lock-range-table-relations-in-the-executor.patch

This removes all instances of heap_open(relid, <not-NoLock>) in the
executor code with heap_open(relid, NoLock).  To verify that an
appropriate lock is already taken on a relation by the time we get into
executor, this also installs an assert-build-only check that confirms that
lmgr indeed holds the lock.  To remember which lock was taken when
creating a given RTE_RELATION range table entry, this adds a lockmode
field to RangeTblEntry.  Finally, because we don't lock in the executor
and hence there are no concerns about lock strength upgrade hazard,
InitPlan doesn't need toinitialize ResultRelInfos and ExecRowMarks, in
favor of doing that in the ExecInit* routines of respective nodes which
need those ResultRelInfos and ExecLockRows.

(This doesn't touch index relations though, as they don't have a range
table entry.)	

0002-Remove-useless-fields-from-planner-nodes.patch

This removes some fields from PlannedStmt whose only purpose currently is
to help InitPlan do certain things that it no longer does, as mentioned
above.  Also, some fields in Append, MergeAppend, ModifyTable, whose only
purpose currently is to propagate partitioned table RT indexes so that
executor could lock them, are removed.  Removing them also means that the
planner doesn't have to spend cycles initializing them.

0003-Prune-PlanRowMark-of-relations-that-are-pruned-from-.patch

This prevents PlanRowMarks corresponding to relations that won't be
scanned by a given plan from appearing in the rowMarks field of LockRows
or ModifyTable nodes.  This results in removing significant overhead from
the executor initialization of those nodes, especially for partitioned
tables with many partitions.

0004-Revise-executor-range-table-relation-opening-closing.patch

This adds two arrays to EState indexed by RT indexes, one for
RangeTblEntry's and another for Relation pointers.  The former reduces the
cost of fetching RangeTblEntry by RT index.  The latter is used by a newly
introduced function ExecRangeTableRelation(), which replaces heap_open for
relations that are contained in the range table.  If a given RTE's
relation is opened by multiple times, only the first call of
ExecRangeTableRelation will go to relcache.



Although improving executor's performance is not the main goal of these
patches, the fact that we're getting rid of redundant processing means
there would be at least some speedup, especially with large number of
relations in the range table, such as with partitioned tables with many
partitions.

* Benchmark script used:

$ cat /tmp/select-lt.sql
select * from lt where b = 999;

'lt' above is a list-partitioned table with 1000 partitions, with one
partition for each value in the range 1..1000.


$ for i in 1 2;
> do
> pgbench -n -Mprepared -T 60 -f /tmp/select-lt.sql
> done

master

tps = 768.172129 (excluding connections establishing)
tps = 764.180834 (excluding connections establishing)

patch 0001 (no locking in the executor)

tps = 775.060323 (excluding connections establishing)
tps = 778.772917 (excluding connections establishing)

patch 0002 (remove useless planner node fields)

tps = 782.165436 (excluding connections establishing)
tps = 759.417411 (excluding connections establishing

patch 0003 (prune PlanRowMarks)

tps = 783.558539 (excluding connections establishing)
tps = 776.106055 (excluding connections establishing)

patch 0004 (executor range table Relation open)

tps = 778.924649 (excluding connections establishing)
tps = 769.093431 (excluding connections establishing)


Speedup is more pronounced with a benchmark that needs RowMarks, because
one of the patches (0003) removes overhead around handling them.

$ cat /tmp/select-lt-for-share.sql
select * from lt where b = 999 for share;

master

tps = 94.095985 (excluding connections establishing)
tps = 93.955702 (excluding connections establishing)

patch 0001 (no locking in the executor)

tps = 199.030555 (excluding connections establishing)
tps = 197.630424 (excluding connections establishing)

patch 0002 (remove useless planner node fields)

tps = 194.384994 (excluding connections establishing)
tps = 195.821362 (excluding connections establishing)

patch 0003 (prune PlanRowMarks)

tps = 712.544029 (excluding connections establishing)
tps = 717.540052 (excluding connections establishing)

patch 0004 (executor range table Relation open)

tps = 725.189715 (excluding connections establishing)
tps = 727.344683 (excluding connections establishing)


Will add this to next CF.

Thanks,
Amit

  2. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-09-04T08:53:11Z

    On 2018/08/16 17:22, Amit Langote wrote:
> 0004-Revise-executor-range-table-relation-opening-closing.patch
> 
> This adds two arrays to EState indexed by RT indexes, one for
> RangeTblEntry's and another for Relation pointers.  The former reduces the
> cost of fetching RangeTblEntry by RT index.  The latter is used by a newly
> introduced function ExecRangeTableRelation(), which replaces heap_open for
> relations that are contained in the range table.  If a given RTE's
> relation is opened by multiple times, only the first call of
> ExecRangeTableRelation will go to relcache.

David Rowely recently, independently [1], proposed one of the ideas
mentioned above (store RangeTblEntry's in an array in EState).  As I
mentioned in reply to his email, I think his implementation of the idea is
better than mine, so I've merged his patch in the above patch, except one
part: instead of removing the es_range_table list altogether, I decided to
keep it around so that ExecSerializePlan doesn't have to build one all
over again from the array like his patch did.

Updated patches attached; 0001-0003 are same as v1.

Thanks,
Amit

[1] Make executor's Range Table an array instead of a List
https://postgr.es/m/CAKJS1f9EypD_=xG6ACFdF=1cBjz+Z9hiHHSd-RqLjor+QyA-nw@mail.gmail.com

  3. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-09-10T04:36:57Z

    On 4 September 2018 at 20:53, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Updated patches attached; 0001-0003 are same as v1.

I've looked at these. Here's my review so far:

0001:

1. The following does not seem to be true any longer:

+ /*
+ * If you change the conditions under which rel locks are acquired
+ * here, be sure to adjust ExecOpenScanRelation to match.
+ */

per:

@@ -652,28 +654,10 @@ ExecOpenScanRelation(EState *estate, Index
scanrelid, int eflags)
 {
  Relation rel;
  Oid reloid;
- LOCKMODE lockmode;

- /*
- * Determine the lock type we need.  First, scan to see if target relation
- * is a result relation.  If not, check if it's a FOR UPDATE/FOR SHARE
- * relation.  In either of those cases, we got the lock already.
- */
- lockmode = AccessShareLock;
- if (ExecRelationIsTargetRelation(estate, scanrelid))
- lockmode = NoLock;
- else
- {
- /* Keep this check in sync with InitPlan! */
- ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true);
-
- if (erm != NULL && erm->relation != NULL)
- lockmode = NoLock;
- }
-

2. Should addRangeTableEntryForRelation() initialize lockmode, or
maybe take it as a parameter?

3. Don't think there's a need to capitalise true and false in:

+ * Return TRUE if we acquired a new lock, FALSE if already held.

4. The comment probably should read "lock level to obtain, or 0 if no
lock is required" in:

+ int lockmode; /* lock taken on the relation or 0 */

The field should likely also be LOCKMODE, not int.

5. AcquireExecutorLocks() does not need the local variable named rt_index

0002:

6. I don't think "rootRelation" is a good name for this field. I think
"root" is being confused with "target". Nothing is it say the target
is the same as the root.

+ Index rootRelation; /* RT index of root partitioned table */

Perhaps "partitionedTarget" is a better name?

7. Should use "else" instead of "else if" in:

+ /* Top-level Plan must be LockRows or ModifyTable */
+ Assert(IsA(stmt->planTree, LockRows) ||
+    IsA(stmt->planTree, ModifyTable));
+ if (IsA(stmt->planTree, LockRows))
+ rowMarks = ((LockRows *) stmt->planTree)->rowMarks;
+ else if (IsA(stmt->planTree, ModifyTable))
+ rowMarks = ((ModifyTable *) stmt->planTree)->rowMarks;

or you'll likely get a compiler warning on non-Assert enabled builds.

0003:

8. The following code seems repeated enough to warrant a static function:

+ rowMarks = NIL;
+ foreach(lc, root->rowMarks)
+ {
+ PlanRowMark *rc = lfirst(lc);
+
+ if (root->simple_rel_array[rc->rti] != NULL &&
+ IS_DUMMY_REL(root->simple_rel_array[rc->rti]))
+ continue;
+
+ rowMarks = lappend(rowMarks, rc);
+ }

Also, why not reverse the condition and do the lappend inside the if?
Save two lines.

9. The following code appears in copy.c, which is pretty much the same
as the code in execMain.c:

estate->es_range_table_size = list_length(cstate->range_table);
estate->es_range_table_array = (RangeTblEntry **)
palloc(sizeof(RangeTblEntry *) *
   estate->es_range_table_size);
/* Populate the range table array */
i = 0;
foreach(lc, cstate->range_table)
estate->es_range_table_array[i++] = lfirst_node(RangeTblEntry, lc);

Would it not be better to invent a function with the signature:

void
setup_range_table_array(EState *estate, List *rangeTable)

and use it in both locations?

10. In create_estate_for_relation() I don't think you should remove
the line that sets es_range_table.

@@ -199,7 +199,8 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel)
  rte->rtekind = RTE_RELATION;
  rte->relid = RelationGetRelid(rel->localrel);
  rte->relkind = rel->localrel->rd_rel->relkind;
- estate->es_range_table = list_make1(rte);

If you're keeping es_range_table then I think it needs to always be
set properly to help prevent future bugs in that area.

11. ExecRangeTableRelation should look more like:

ExecRangeTableRelation(EState *estate, Index rti)
{
  Relation rel = estate->es_relations[rti - 1];

  if (rel != NULL)
    RelationIncrementReferenceCount(rel);
  else
  {
    RangeTblEntry *rte = exec_rt_fetch(rti, estate->es_range_table_array);

   /*
    * No need to lock the relation lock, because upstream code
    * must hold the lock already.
    */
    rel = estate->es_relations[rti - 1] = heap_open(rte->relid, NoLock);
  }

  return rel;
}

12. I think this should read: /* Fill in RTEs. es_relations will be
populated later. */

+ /* Fill the RTEs, Relations array will be filled later. */

13. I also notice that you're still cleaning up Relations with
heap_close or relation_close. Did you consider not doing that and just
having a function that's run at the end of execution which closes all
non-NULL es_relations? This way you'd not need to perform
RelationIncrementReferenceCount inside ExecRangeTableRelation.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  4. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-09-12T05:23:57Z

    Thank you for reviewing.

On 2018/09/10 13:36, David Rowley wrote:
> On 4 September 2018 at 20:53, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Updated patches attached; 0001-0003 are same as v1.
> 
> I've looked at these. Here's my review so far:
> 
> 0001:
> 
> 1. The following does not seem to be true any longer:
> 
> + /*
> + * If you change the conditions under which rel locks are acquired
> + * here, be sure to adjust ExecOpenScanRelation to match.
> + */
> 
> per:
> 
> @@ -652,28 +654,10 @@ ExecOpenScanRelation(EState *estate, Index
> scanrelid, int eflags)
>  {
>   Relation rel;
>   Oid reloid;
> - LOCKMODE lockmode;
> 
> - /*
> - * Determine the lock type we need.  First, scan to see if target relation
> - * is a result relation.  If not, check if it's a FOR UPDATE/FOR SHARE
> - * relation.  In either of those cases, we got the lock already.
> - */
> - lockmode = AccessShareLock;
> - if (ExecRelationIsTargetRelation(estate, scanrelid))
> - lockmode = NoLock;
> - else
> - {
> - /* Keep this check in sync with InitPlan! */
> - ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true);
> -
> - if (erm != NULL && erm->relation != NULL)
> - lockmode = NoLock;
> - }
> -

Yeah, removed that comment in ExecBuildRowMark.

> 2. Should addRangeTableEntryForRelation() initialize lockmode, or
> maybe take it as a parameter?

Hmm, that sounds like a good idea.  Looking at the various places it's
called from though, it's not clear in many instances which lock was taken
on the relation that's passed to it, because the lock itself seems to be
taken several stack frames removed from the call site.

That said, at least for the cases that we care about, that is, the cases
in which the RTE being built will be passed to the executor by the way of
being in a PlannedStmt, it's clear which lock is taken.  So, we can add
the lockmode parameter to addRangeTableEntryForRelation as you suggest and
pass the actual lockmode value only in the cases we care about, mentioning
the fact in the function's header comment that callers may pass NoLock
arbitrarily if it's clear the RTE won't be passed to the executor.

I've modified patch that way.  Thoughts?

> 3. Don't think there's a need to capitalise true and false in:
> 
> + * Return TRUE if we acquired a new lock, FALSE if already held.

OK, fixed.

> 4. The comment probably should read "lock level to obtain, or 0 if no
> lock is required" in:
> 
> + int lockmode; /* lock taken on the relation or 0 */

OK.

> The field should likely also be LOCKMODE, not int.

OK.

> 5. AcquireExecutorLocks() does not need the local variable named rt_index

Good catch, removed.

> 0002:
> 
> 6. I don't think "rootRelation" is a good name for this field. I think
> "root" is being confused with "target". Nothing is it say the target
> is the same as the root.
> 
> + Index rootRelation; /* RT index of root partitioned table */
> 
> Perhaps "partitionedTarget" is a better name?

I realized that we don't need a new Index field here.  nominalRelation
serves the purpose that rootRelation is meant for, so it seems silly to
have two fields of the same value.  Instead, let's have a bool
partitionedTarget which is set to true if the target (whose RT index is
nominalRelation) is a partitioned tables.

> 7. Should use "else" instead of "else if" in:
> 
> + /* Top-level Plan must be LockRows or ModifyTable */
> + Assert(IsA(stmt->planTree, LockRows) ||
> +    IsA(stmt->planTree, ModifyTable));
> + if (IsA(stmt->planTree, LockRows))
> + rowMarks = ((LockRows *) stmt->planTree)->rowMarks;
> + else if (IsA(stmt->planTree, ModifyTable))
> + rowMarks = ((ModifyTable *) stmt->planTree)->rowMarks;
> 
> or you'll likely get a compiler warning on non-Assert enabled builds.

Yep, fixed.

> 0003:
> 
> 8. The following code seems repeated enough to warrant a static function:
> 
> + rowMarks = NIL;
> + foreach(lc, root->rowMarks)
> + {
> + PlanRowMark *rc = lfirst(lc);
> +
> + if (root->simple_rel_array[rc->rti] != NULL &&
> + IS_DUMMY_REL(root->simple_rel_array[rc->rti]))
> + continue;
> +
> + rowMarks = lappend(rowMarks, rc);
> + }
> 
> Also, why not reverse the condition and do the lappend inside the if?
> Save two lines.

OK, made this change and added a static function called
get_unpruned_rowmarks(PlannerInfo *root).

> 
> 9. The following code appears in copy.c, which is pretty much the same
> as the code in execMain.c:
> 
> estate->es_range_table_size = list_length(cstate->range_table);
> estate->es_range_table_array = (RangeTblEntry **)
> palloc(sizeof(RangeTblEntry *) *
>    estate->es_range_table_size);
> /* Populate the range table array */
> i = 0;
> foreach(lc, cstate->range_table)
> estate->es_range_table_array[i++] = lfirst_node(RangeTblEntry, lc);
> 
> Would it not be better to invent a function with the signature:
> 
> void
> setup_range_table_array(EState *estate, List *rangeTable)
> 
> and use it in both locations?

Agreed, but I named it ExecInitRangeTable.

> 10. In create_estate_for_relation() I don't think you should remove
> the line that sets es_range_table.
> 
> @@ -199,7 +199,8 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel)
>   rte->rtekind = RTE_RELATION;
>   rte->relid = RelationGetRelid(rel->localrel);
>   rte->relkind = rel->localrel->rd_rel->relkind;
> - estate->es_range_table = list_make1(rte);
> 
> If you're keeping es_range_table then I think it needs to always be
> set properly to help prevent future bugs in that area.

My bad, fixed.

> 11. ExecRangeTableRelation should look more like:
> 
> ExecRangeTableRelation(EState *estate, Index rti)
> {
>   Relation rel = estate->es_relations[rti - 1];
> 
>   if (rel != NULL)
>     RelationIncrementReferenceCount(rel);
>   else
>   {
>     RangeTblEntry *rte = exec_rt_fetch(rti, estate->es_range_table_array);
> 
>    /*
>     * No need to lock the relation lock, because upstream code
>     * must hold the lock already.
>     */
>     rel = estate->es_relations[rti - 1] = heap_open(rte->relid, NoLock);
>   }
> 
>   return rel;
> }

Much better, done.

> 12. I think this should read: /* Fill in RTEs. es_relations will be
> populated later. */
> 
> + /* Fill the RTEs, Relations array will be filled later. */

I've rewritten the comment.

> 13. I also notice that you're still cleaning up Relations with
> heap_close or relation_close. Did you consider not doing that and just
> having a function that's run at the end of execution which closes all
> non-NULL es_relations? This way you'd not need to perform
> RelationIncrementReferenceCount inside ExecRangeTableRelation.

Agreed, done.  I'd be slightly hesitant to remove ExecCloseScanRelation,
ExecDestroyPartitionPruneState et al if they weren't just a wrapper around
heap_close.

Please find attached revised patches.

Thanks,
Amit

  5. Re: executor relation handling

    Jesper Pedersen <jesper.pedersen@redhat.com> — 2018-09-12T12:23:24Z

    Hi Amit,

On 9/12/18 1:23 AM, Amit Langote wrote:
> Please find attached revised patches.
> 

After applying 0004 I'm getting a crash in 'eval-plan-qual' during 
check-world using

export CFLAGS="-DCOPY_PARSE_PLAN_TREES -O0 -fno-omit-frame-pointer" && 
./configure --enable-dtrace --with-openssl --with-gssapi --with-libxml 
--with-llvm --enable-debug --enable-depend --enable-tap-tests 
--enable-cassert

Confirmed by CFBot in [1].

[1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/427530296

Best regards,
  Jesper

  6. Re: executor relation handling

    Amit Langote <amitlangote09@gmail.com> — 2018-09-12T15:23:48Z

    On Wed, Sep 12, 2018 at 9:23 PM, Jesper Pedersen
<jesper.pedersen@redhat.com> wrote:
> Hi Amit,
>
> On 9/12/18 1:23 AM, Amit Langote wrote:
>>
>> Please find attached revised patches.
>>
>
> After applying 0004 I'm getting a crash in 'eval-plan-qual' during
> check-world using
>
> export CFLAGS="-DCOPY_PARSE_PLAN_TREES -O0 -fno-omit-frame-pointer" &&
> ./configure --enable-dtrace --with-openssl --with-gssapi --with-libxml
> --with-llvm --enable-debug --enable-depend --enable-tap-tests
> --enable-cassert
>
> Confirmed by CFBot in [1].
>
> [1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/427530296

Thanks Jesper.  Will look into it first thing tomorrow morning.

Thanks,
Amit

  7. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-09-13T04:58:05Z

    On 2018/09/13 0:23, Amit Langote wrote:
> On Wed, Sep 12, 2018 at 9:23 PM, Jesper Pedersen
> <jesper.pedersen@redhat.com> wrote:
>> Hi Amit,
>>
>> On 9/12/18 1:23 AM, Amit Langote wrote:
>>>
>>> Please find attached revised patches.
>>>
>>
>> After applying 0004 I'm getting a crash in 'eval-plan-qual' during
>> check-world using
>>
>> export CFLAGS="-DCOPY_PARSE_PLAN_TREES -O0 -fno-omit-frame-pointer" &&
>> ./configure --enable-dtrace --with-openssl --with-gssapi --with-libxml
>> --with-llvm --enable-debug --enable-depend --enable-tap-tests
>> --enable-cassert
>>
>> Confirmed by CFBot in [1].
>>
>> [1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/427530296
> 
> Thanks Jesper.  Will look into it first thing tomorrow morning.

Attached updated patches.

Beside the issue that caused eval-plan-qual isolation test to crash, I
also spotted and fixed an oversight in the 0002 patch which would lead to
EState.es_output_cid being set to wrong value and causing unexpected error
during tuple locking as result of that.

Thanks,
Amit

  8. Re: executor relation handling

    Jesper Pedersen <jesper.pedersen@redhat.com> — 2018-09-13T11:27:45Z

    Hi Amit,

On 9/13/18 12:58 AM, Amit Langote wrote:
> Attached updated patches.
> 
> Beside the issue that caused eval-plan-qual isolation test to crash, I
> also spotted and fixed an oversight in the 0002 patch which would lead to
> EState.es_output_cid being set to wrong value and causing unexpected error
> during tuple locking as result of that.
> 

Thanks for the update.

However, the subscription TAP test 
(src/test/subscription/t/001_rep_changes.pl) is still failing.

CFBot has the same log

  https://travis-ci.org/postgresql-cfbot/postgresql/builds/427999969

as locally.

Best regards,
  Jesper

  9. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-09-14T05:36:38Z

    Thanks again, Jesper.

On 2018/09/13 20:27, Jesper Pedersen wrote:
> Hi Amit,
> 
> On 9/13/18 12:58 AM, Amit Langote wrote:
>> Attached updated patches.
>>
>> Beside the issue that caused eval-plan-qual isolation test to crash, I
>> also spotted and fixed an oversight in the 0002 patch which would lead to
>> EState.es_output_cid being set to wrong value and causing unexpected error
>> during tuple locking as result of that.
>>
> 
> Thanks for the update.
> 
> However, the subscription TAP test
> (src/test/subscription/t/001_rep_changes.pl) is still failing.
> 
> CFBot has the same log
> 
>  https://travis-ci.org/postgresql-cfbot/postgresql/builds/427999969
> 
> as locally.

My bad.  I missed that logical replication code depends on the affected
executor code.

Fixed patches attached.

Thanks,
Amit

  10. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-09-27T09:15:50Z

    I've just completed a review of the v5 patch set. I ended up just
making the changes myself since Amit mentioned he was on leave for a
few weeks.

Summary of changes:

1. Changed the way we verify the lock already exists with debug
builds. I reverted some incorrect code added to LockRelationOid that
seems to have gotten broken after being rebased on f868a8143a9.  I've
just added some functions that verify the lock is in the
LockMethodLocalHash hashtable.
2. Fixed some incorrect lock types being passed into
addRangeTableEntryForRelation()
3. Added code in addRangeTableEntryForRelation to verify we actually
hold the lock that the parameter claims we do. (This found all the
errors I fixed in #2)
4. Updated various comments outdated by the patches
5. Updated executor README's mention that we close relations when
calling the end node function.  This is now handled at the end of
execution.
6. Renamed nominalRelation to targetRelation.  I think this fits
better since we're overloading the variable.
7. Use LOCKMODE instead of int in some places.
8. Changed warning about relation not locked to WARNING instead of NOTICE.
9. Renamed get_unpruned_rowmarks() to get_nondummy_rowmarks(). Pruning
makes me think of partition pruning but the function checks for dummy
rels. These could be dummy for reasons other than partition pruning.

I've attached a diff showing the changes I made along with the full
patches which I tagged as v6.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  11. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-09-27T09:23:13Z

    On 2018/09/27 18:15, David Rowley wrote:
> I've just completed a review of the v5 patch set. I ended up just
> making the changes myself since Amit mentioned he was on leave for a
> few weeks.

Thanks David.  I'm back today and will look at the updated patches tomorrow.

Regards,
Amit

  12. Re: executor relation handling

    Jesper Pedersen <jesper.pedersen@redhat.com> — 2018-09-27T14:24:46Z

    Hi,

On 9/27/18 5:15 AM, David Rowley wrote:
> I've just completed a review of the v5 patch set. I ended up just
> making the changes myself since Amit mentioned he was on leave for a
> few weeks.
> 
> Summary of changes:
> 
> 1. Changed the way we verify the lock already exists with debug
> builds. I reverted some incorrect code added to LockRelationOid that
> seems to have gotten broken after being rebased on f868a8143a9.  I've
> just added some functions that verify the lock is in the
> LockMethodLocalHash hashtable.
> 2. Fixed some incorrect lock types being passed into
> addRangeTableEntryForRelation()
> 3. Added code in addRangeTableEntryForRelation to verify we actually
> hold the lock that the parameter claims we do. (This found all the
> errors I fixed in #2)
> 4. Updated various comments outdated by the patches
> 5. Updated executor README's mention that we close relations when
> calling the end node function.  This is now handled at the end of
> execution.
> 6. Renamed nominalRelation to targetRelation.  I think this fits
> better since we're overloading the variable.
> 7. Use LOCKMODE instead of int in some places.
> 8. Changed warning about relation not locked to WARNING instead of NOTICE.
> 9. Renamed get_unpruned_rowmarks() to get_nondummy_rowmarks(). Pruning
> makes me think of partition pruning but the function checks for dummy
> rels. These could be dummy for reasons other than partition pruning.
> 
> I've attached a diff showing the changes I made along with the full
> patches which I tagged as v6.
> 

Thanks David and Amit -- this version passes check-world. I'll also take 
a deeper look.

Best regards,
  Jesper

  13. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-09-28T08:00:00Z

    On 2018/09/27 18:15, David Rowley wrote:
> I've just completed a review of the v5 patch set. I ended up just
> making the changes myself since Amit mentioned he was on leave for a
> few weeks.
> 
> Summary of changes:
> 
> 1. Changed the way we verify the lock already exists with debug
> builds. I reverted some incorrect code added to LockRelationOid that
> seems to have gotten broken after being rebased on f868a8143a9.  I've
> just added some functions that verify the lock is in the
> LockMethodLocalHash hashtable.

Thanks.  I guess I wasn't terribly happy with my job of rebasing on top of
f868a8143a9, because that commit had made the result of
LockAcquireExtended a bit ambiguous for me.

I like the new CheckRelationLockedByUs() and LocalLockExists() functions
that you added to lock manager.

> 2. Fixed some incorrect lock types being passed into
> addRangeTableEntryForRelation()
>
> 3. Added code in addRangeTableEntryForRelation to verify we actually
> hold the lock that the parameter claims we do. (This found all the
> errors I fixed in #2)

I see that I'd falsely copy-pasted AccessShareLock in transformRuleStmt,
which you've corrected to AccessExclusiveLock.  I was not sure about other
cases where you've replaced NoLock by something else, because they won't
be checked or asserted, but perhaps that's fine.

> 4. Updated various comments outdated by the patches
> 5. Updated executor README's mention that we close relations when
> calling the end node function.  This is now handled at the end of
> execution.

Thank you.  I see that you also fixed some useless code that I had left
lying around as result of code movement such as the following dead code:

@@ -2711,9 +2711,7 @@ ExecEndModifyTable(ModifyTableState *node)
 {
     int         i;

-    /*
-     * close the result relation(s) if any, but hold locks until xact commit.
-     */
+    /* Perform cleanup. */
     for (i = 0; i < node->mt_nplans; i++)
     {
         ResultRelInfo *resultRelInfo = node->resultRelInfo + i;
@@ -2728,7 +2726,6 @@ ExecEndModifyTable(ModifyTableState *node)
         /* Close indices and then the relation itself */
         ExecCloseIndices(resultRelInfo);
         heap_close(resultRelInfo->ri_RelationDesc, NoLock);
-        resultRelInfo++;
     }

> 6. Renamed nominalRelation to targetRelation.  I think this fits
> better since we're overloading the variable.

That makes sense to me.

> 7. Use LOCKMODE instead of int in some places.
> 8. Changed warning about relation not locked to WARNING instead of NOTICE.

Oops, think I'd forgotten to do that myself.

> 9. Renamed get_unpruned_rowmarks() to get_nondummy_rowmarks(). Pruning
> makes me think of partition pruning but the function checks for dummy
> rels. These could be dummy for reasons other than partition pruning.

Makes sense too.

> I've attached a diff showing the changes I made along with the full
> patches which I tagged as v6.

Thanks a lot for working on that.  I've made minor tweaks, which find in
the attached updated patches (a .diff file containing changes from v6 to
v7 is also attached).

Regards,
Amit

  14. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-09-28T08:21:02Z

    On 28 September 2018 at 20:00, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> I've made minor tweaks, which find in
> the attached updated patches (a .diff file containing changes from v6 to
> v7 is also attached).

Thanks for looking over the changes.

I've looked at the v6 to v7 diff and it seems all good, apart from:

+ * The following asserts that the necessary lock on the relation

I think we maybe should switch the word "assert" for "verifies". The
Assert is just checking we didn't get a NoLock and I don't think
you're using "assert" meaning the Assert() marco, so likely should be
changed to avoid confusion.

Apart from that, I see nothing wrong with the patches, so I think we
should get someone else to look. I'm marking it as ready for
committer.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  15. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-09-28T08:28:58Z

    On 2018/09/28 17:21, David Rowley wrote:
> On 28 September 2018 at 20:00, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> I've made minor tweaks, which find in
>> the attached updated patches (a .diff file containing changes from v6 to
>> v7 is also attached).
> 
> Thanks for looking over the changes.
> 
> I've looked at the v6 to v7 diff and it seems all good, apart from:
> 
> + * The following asserts that the necessary lock on the relation
> 
> I think we maybe should switch the word "assert" for "verifies". The
> Assert is just checking we didn't get a NoLock and I don't think
> you're using "assert" meaning the Assert() marco, so likely should be
> changed to avoid confusion.

Okay, I've revised the text in the attached updated patch.

> Apart from that, I see nothing wrong with the patches, so I think we
> should get someone else to look. I'm marking it as ready for
> committer.

Thanks for your time reviewing the patches.

Regards,
Amit

  16. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-09-28T08:48:33Z

    On 28 September 2018 at 20:28, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2018/09/28 17:21, David Rowley wrote:
>> I think we maybe should switch the word "assert" for "verifies". The
>> Assert is just checking we didn't get a NoLock and I don't think
>> you're using "assert" meaning the Assert() marco, so likely should be
>> changed to avoid confusion.
>
> Okay, I've revised the text in the attached updated patch.

Meh, I just noticed that the WARNING text claims "InitPlan" is the
function name. I think it's best to get rid of that. It's pretty much
redundant anyway if you do: \set VERBOSITY verbose

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  17. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-09-28T08:58:49Z

    On 2018/09/28 17:48, David Rowley wrote:
> On 28 September 2018 at 20:28, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2018/09/28 17:21, David Rowley wrote:
>>> I think we maybe should switch the word "assert" for "verifies". The
>>> Assert is just checking we didn't get a NoLock and I don't think
>>> you're using "assert" meaning the Assert() marco, so likely should be
>>> changed to avoid confusion.
>>
>> Okay, I've revised the text in the attached updated patch.
> 
> Meh, I just noticed that the WARNING text claims "InitPlan" is the
> function name. I think it's best to get rid of that. It's pretty much
> redundant anyway if you do: \set VERBOSITY verbose

Oops, good catch that one.  Removed "InitPlan: " from the message in the
attached.

Thanks,
Amit

  18. Re: executor relation handling

    Jesper Pedersen <jesper.pedersen@redhat.com> — 2018-09-28T13:51:47Z

    Hi,

On 9/28/18 4:58 AM, Amit Langote wrote:
>>> Okay, I've revised the text in the attached updated patch.
>>
>> Meh, I just noticed that the WARNING text claims "InitPlan" is the
>> function name. I think it's best to get rid of that. It's pretty much
>> redundant anyway if you do: \set VERBOSITY verbose
> 
> Oops, good catch that one.  Removed "InitPlan: " from the message in the
> attached.
> 

I have looked at the patch (v9), and have no further comments. I can 
confirm a speedup in the SELECT FOR SHARE case.

Thanks for working on this !

Best regards,
  Jesper

  19. Re: executor relation handling

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-09-28T15:49:46Z

    On 2018-Sep-28, Amit Langote wrote:
> On 2018/09/28 17:48, David Rowley wrote:

> > Meh, I just noticed that the WARNING text claims "InitPlan" is the
> > function name. I think it's best to get rid of that. It's pretty much
> > redundant anyway if you do: \set VERBOSITY verbose
> 
> Oops, good catch that one.  Removed "InitPlan: " from the message in the
> attached.

Were there two cases of that?  Because one still remains.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  20. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-09-29T00:46:36Z

    On 29 September 2018 at 03:49, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2018-Sep-28, Amit Langote wrote:
>> On 2018/09/28 17:48, David Rowley wrote:
>
>> > Meh, I just noticed that the WARNING text claims "InitPlan" is the
>> > function name. I think it's best to get rid of that. It's pretty much
>> > redundant anyway if you do: \set VERBOSITY verbose
>>
>> Oops, good catch that one.  Removed "InitPlan: " from the message in the
>> attached.
>
> Were there two cases of that?  Because one still remains.

Yeah, 0001 added it and 0004 removed it again replacing it with the
corrected version.

I've attached v10 which fixes this and aligns the WARNING text in
ExecInitRangeTable() and addRangeTableEntryForRelation().

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  21. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-29T20:04:17Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> I've attached v10 which fixes this and aligns the WARNING text in
> ExecInitRangeTable() and addRangeTableEntryForRelation().

I started poking at this.  I thought that it would be a good cross-check
to apply just the "front half" of 0001 (i.e., creation and population of
the RTE lockmode field), and then to insert checks in each of the
"back half" places (executor, plancache, etc) that the lockmodes they
are computing today match what is in the RTE.

Those checks fell over many times in the regression tests.

There seem to be at least four distinct problems:

1. You set up transformRuleStmt to insert AccessExclusiveLock into
the "OLD" and "NEW" RTEs for a view.  This is surely wrong; we do
not want to take exclusive lock on a view just to run a query using
the view.  It should (usually, anyway) just be AccessShareLock.
However, because addRangeTableEntryForRelation insists that you
hold the requested lock type *now*, just changing the parameter
to AccessShareLock doesn't work.

I hacked around this for the moment by passing NoLock to
addRangeTableEntryForRelation and then changing rte->lockmode
after it returns, but man that's ugly.  It makes me wonder whether
addRangeTableEntryForRelation should be checking the lockmode at all.

I'm inclined to think we should remove the check for current lockmode
in addRangeTableEntryForRelation, and instead just assert that the
passed lockmode must be one of AccessShareLock, RowShareLock, or
RowExclusiveLock depending on whether the RTE is meant to represent
a plain source table, a FOR UPDATE/SHARE source table, or a target
table.  I don't think it's that helpful to be checking that the
caller got exactly the same lock, especially given the number of
places where the patch had to cheat already by using NoLock.

2. The "forUpdatePushedDown" override in AcquireRewriteLocks isn't
getting modeled in the RTE lockmode fields.  In other words, if we
have something like

	CREATE VIEW vv AS SELECT * FROM tab1;
	SELECT * FROM vv FOR UPDATE OF vv;

the checks fall over, because the tab1 RTE in the view's rangetable
just has AccessShareLock, but we need RowShareLock.  I fixed this
by having AcquireRewriteLocks actually modify the RTE if it is
promoting the lock level.  This is kinda grotty, but we were already
assuming that AcquireRewriteLocks could scribble on the query tree,
so it seems safe enough.

3. There remain some cases where the RTE says RowExclusiveLock but
the executor calculation indicates we only need AccessShareLock.
AFAICT, this happens only when we have a DO ALSO rule that results
in an added query that merely scans the target table.  The RTE used
for that purpose is just the original one, so it still has a lockmode
suitable for a target table.

We could probably hack the rewriter so that it changes the RTE lockmode
back down to AccessShareLock in these cases.  However, I'm inclined to
think that that is something *not* to do, and that we should let the
higher lockmode be used instead, for two reasons:

(1) Asking for both AccessShareLock and RowExclusiveLock on the same
table requires an extra trip through the shared lock manager, for little
value that I can see.

(2) If the DO ALSO rule is run before the main one, we'd be acquiring
AccessShareLock before RowExclusiveLock, resulting in deadlock hazard
due to lock upgrade.  (I think this may be a pre-existing bug, although
it could likely only manifest in corner cases such as where we're pulling
a plan tree out of plancache.  In most cases the first thing we'd acquire
on a rule target table is RowExclusiveLock in the parser, before any
rule rewriting could happen.)

4. I also notice some cases (all in FDW tests) where ExecOpenScanRelation
is choosing AccessShareLock although the RTE has RowShareLock.  These seem
to all be cases where we're implementing FOR UPDATE/SHARE via
ROW_MARK_COPY, so that this:

		ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true);

		if (erm != NULL && erm->relation != NULL)
			lockmode = NoLock;

leaves lockmode as AccessShareLock because erm->relation is NULL.
Again, I think this is probably OK, and it'd be better to use
RowShareLock for consistency with other places that might open
the rel.  It will be a change in externally-visible behavior though.

Thoughts?

			regards, tom lane

  22. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-29T22:05:41Z

    I wrote:
> I started poking at this.  I thought that it would be a good cross-check
> to apply just the "front half" of 0001 (i.e., creation and population of
> the RTE lockmode field), and then to insert checks in each of the
> "back half" places (executor, plancache, etc) that the lockmodes they
> are computing today match what is in the RTE.
> Those checks fell over many times in the regression tests.

Here's a version that doesn't fall over (for me), incorporating fixes
as I suggested for points 1 and 2, and weakening the back-half assertions
enough to let points 3 and 4 succeed.  I'm inclined to commit this to see
if the buildfarm can find any problems I missed.

Some cosmetic changes:

* I renamed the RTE field to rellockmode in the name of greppability.

* I rearranged the order of the arguments for
addRangeTableEntryForRelation to make it more consistent with the
other addRangeTableEntryXXX functions.

			regards, tom lane

  23. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-30T17:18:14Z

    I wrote:
> 1. You set up transformRuleStmt to insert AccessExclusiveLock into
> the "OLD" and "NEW" RTEs for a view.  This is surely wrong; we do
> not want to take exclusive lock on a view just to run a query using
> the view.  It should (usually, anyway) just be AccessShareLock.
> However, because addRangeTableEntryForRelation insists that you
> hold the requested lock type *now*, just changing the parameter
> to AccessShareLock doesn't work.
> I hacked around this for the moment by passing NoLock to
> addRangeTableEntryForRelation and then changing rte->lockmode
> after it returns, but man that's ugly.  It makes me wonder whether
> addRangeTableEntryForRelation should be checking the lockmode at all.

It occurred to me that it'd be reasonable to insist that the caller
holds a lock *at least as strong* as the one being recorded in the RTE,
and that there's also been discussions about verifying that some lock
is held when something like heap_open(foo, NoLock) is attempted.
So I dusted off the part of 0001 that did that, producing the
attached delta patch.

Unfortunately, I can't commit this, because it exposes at least two
pre-existing bugs :-(.  So we'll need to fix those first, which seems
like it should be a separate thread.  I'm just parking this here for
the moment.

I think that the call sites should ultimately look like

	Assert(CheckRelationLockedByMe(...));

but for hunting down the places where the assertion currently fails,
it's more convenient if it's just an elog(WARNING).

			regards, tom lane

  24. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-09-30T22:54:27Z

    On 1 October 2018 at 06:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It occurred to me that it'd be reasonable to insist that the caller
> holds a lock *at least as strong* as the one being recorded in the RTE,
> and that there's also been discussions about verifying that some lock
> is held when something like heap_open(foo, NoLock) is attempted.
> So I dusted off the part of 0001 that did that, producing the
> attached delta patch.

My imagination struggles to think of a case, but perhaps one day in
the future we might have a lock manager that coordinates locks on
multiple nodes. If so, is there not a risk that one day we might have
a lock level greater than AccessExclusiveLock, meaning the following
would get broken:

+ for (slockmode = lockmode + 1;
+ slockmode <= AccessExclusiveLock;
+ slockmode++)

For index strategies we do:

#define BTGreaterStrategyNumber 5

#define BTMaxStrategyNumber 5

So would it not be better to add the following to lockdefs.h?

#define MaxLockLevel 8

then use that to terminate the loop.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  25. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T02:49:18Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> On 1 October 2018 at 06:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> + for (slockmode = lockmode + 1;
>> +      slockmode <= AccessExclusiveLock;
>> +      slockmode++)

> So would it not be better to add the following to lockdefs.h?
> #define MaxLockLevel 8
> then use that to terminate the loop.

Good idea, will do.

			regards, tom lane

  26. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-01T06:39:22Z

    On 2018/09/30 5:04, Tom Lane wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> I've attached v10 which fixes this and aligns the WARNING text in
>> ExecInitRangeTable() and addRangeTableEntryForRelation().
> 
> I started poking at this.

Thanks a lot for looking at this.

> I thought that it would be a good cross-check
> to apply just the "front half" of 0001 (i.e., creation and population of
> the RTE lockmode field), and then to insert checks in each of the
> "back half" places (executor, plancache, etc) that the lockmodes they
> are computing today match what is in the RTE.
> 
> Those checks fell over many times in the regression tests.
> 
> There seem to be at least four distinct problems:
> 
> 1. You set up transformRuleStmt to insert AccessExclusiveLock into
> the "OLD" and "NEW" RTEs for a view.  This is surely wrong; we do
> not want to take exclusive lock on a view just to run a query using
> the view.  It should (usually, anyway) just be AccessShareLock.
>
> However, because addRangeTableEntryForRelation insists that you
> hold the requested lock type *now*, just changing the parameter
> to AccessShareLock doesn't work.
>
> I hacked around this for the moment by passing NoLock to
> addRangeTableEntryForRelation and then changing rte->lockmode
> after it returns, but man that's ugly.  It makes me wonder whether
> addRangeTableEntryForRelation should be checking the lockmode at all.
> 
> I'm inclined to think we should remove the check for current lockmode
> in addRangeTableEntryForRelation, and instead just assert that the
> passed lockmode must be one of AccessShareLock, RowShareLock, or
> RowExclusiveLock depending on whether the RTE is meant to represent
> a plain source table, a FOR UPDATE/SHARE source table, or a target
> table.  I don't think it's that helpful to be checking that the
> caller got exactly the same lock, especially given the number of
> places where the patch had to cheat already by using NoLock.

Yeah, addRangeTableEntryForRelation should not insist on holding the
"exact" lock, but rather *at least* as strong as the passed in lock mode.
I see that the patch you posted downthread does that.

In the original patch, the lock mode used for handling the CREATE RULE
command was being conflated with the lock mode to require on NEW and OLD
RTEs that will be added to the rule's query.

> 2. The "forUpdatePushedDown" override in AcquireRewriteLocks isn't
> getting modeled in the RTE lockmode fields.  In other words, if we
> have something like
> 
> 	CREATE VIEW vv AS SELECT * FROM tab1;
> 	SELECT * FROM vv FOR UPDATE OF vv;
> 
> the checks fall over, because the tab1 RTE in the view's rangetable
> just has AccessShareLock, but we need RowShareLock.  I fixed this
> by having AcquireRewriteLocks actually modify the RTE if it is
> promoting the lock level.  This is kinda grotty, but we were already
> assuming that AcquireRewriteLocks could scribble on the query tree,
> so it seems safe enough.

Thanks for fixing that.

> 3. There remain some cases where the RTE says RowExclusiveLock but
> the executor calculation indicates we only need AccessShareLock.
> AFAICT, this happens only when we have a DO ALSO rule that results
> in an added query that merely scans the target table.

I've seen something like that happen for ON CONFLICT's excluded
pseudo-relation RTE too, because the executor deems only the RTE fetched
with result relation RT index to require RowExclusiveLock.

> The RTE used
> for that purpose is just the original one, so it still has a lockmode
> suitable for a target table.
> 
> We could probably hack the rewriter so that it changes the RTE lockmode
> back down to AccessShareLock in these cases.

Is the problematic RTE one coming from an action query?  If so, isn't it
distinct from the original RTE and its rellockmode independently
determined based on whether the action is select or not?

I'm not sure if I understand why we'd need to change its lock mode.

> However, I'm inclined to
> think that that is something *not* to do, and that we should let the
> higher lockmode be used instead, for two reasons:
> 
> (1) Asking for both AccessShareLock and RowExclusiveLock on the same
> table requires an extra trip through the shared lock manager, for little
> value that I can see.
> 
> (2) If the DO ALSO rule is run before the main one, we'd be acquiring
> AccessShareLock before RowExclusiveLock, resulting in deadlock hazard
> due to lock upgrade.  (I think this may be a pre-existing bug, although
> it could likely only manifest in corner cases such as where we're pulling
> a plan tree out of plancache.  In most cases the first thing we'd acquire
> on a rule target table is RowExclusiveLock in the parser, before any
> rule rewriting could happen.)
>
> 4. I also notice some cases (all in FDW tests) where ExecOpenScanRelation
> is choosing AccessShareLock although the RTE has RowShareLock.  These seem
> to all be cases where we're implementing FOR UPDATE/SHARE via
> ROW_MARK_COPY, so that this:
> 
> 		ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true);
> 
> 		if (erm != NULL && erm->relation != NULL)
> 			lockmode = NoLock;
> 
> leaves lockmode as AccessShareLock because erm->relation is NULL.
> Again, I think this is probably OK, and it'd be better to use
> RowShareLock for consistency with other places that might open
> the rel.  It will be a change in externally-visible behavior though.

For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
etc.), I wonder whether we couldn't just *not* recalculate the lock mode
based on inspecting the query tree to cross-check with rellockmode?  Why
not just use rellockmode for locking?  Maybe, okay to keep doing that in
debug builds though.  Also, are the discrepancies like this to be
considered bugs of the existing logic?

Thanks,
Amit

  27. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-01T10:20:29Z

    On 2018/10/01 2:18, Tom Lane wrote:
> I wrote:
>> 1. You set up transformRuleStmt to insert AccessExclusiveLock into
>> the "OLD" and "NEW" RTEs for a view.  This is surely wrong; we do
>> not want to take exclusive lock on a view just to run a query using
>> the view.  It should (usually, anyway) just be AccessShareLock.
>> However, because addRangeTableEntryForRelation insists that you
>> hold the requested lock type *now*, just changing the parameter
>> to AccessShareLock doesn't work.
>> I hacked around this for the moment by passing NoLock to
>> addRangeTableEntryForRelation and then changing rte->lockmode
>> after it returns, but man that's ugly.  It makes me wonder whether
>> addRangeTableEntryForRelation should be checking the lockmode at all.
> 
> It occurred to me that it'd be reasonable to insist that the caller
> holds a lock *at least as strong* as the one being recorded in the RTE,
> and that there's also been discussions about verifying that some lock
> is held when something like heap_open(foo, NoLock) is attempted.
> So I dusted off the part of 0001 that did that, producing the
> attached delta patch.
> 
> Unfortunately, I can't commit this, because it exposes at least two
> pre-existing bugs :-(.  So we'll need to fix those first, which seems
> like it should be a separate thread.  I'm just parking this here for
> the moment.
> 
> I think that the call sites should ultimately look like
> 
> 	Assert(CheckRelationLockedByMe(...));
> 
> but for hunting down the places where the assertion currently fails,
> it's more convenient if it's just an elog(WARNING).

Should this check that we're not in a parallel worker process?

Thanks,
Amit

  28. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T13:45:36Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/09/30 5:04, Tom Lane wrote:
>> 3. There remain some cases where the RTE says RowExclusiveLock but
>> the executor calculation indicates we only need AccessShareLock.
>> AFAICT, this happens only when we have a DO ALSO rule that results
>> in an added query that merely scans the target table.

> I've seen something like that happen for ON CONFLICT's excluded
> pseudo-relation RTE too, because the executor deems only the RTE fetched
> with result relation RT index to require RowExclusiveLock.

OK, I had not carefully inspected every case.

> For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
> etc.), I wonder whether we couldn't just *not* recalculate the lock mode
> based on inspecting the query tree to cross-check with rellockmode?  Why
> not just use rellockmode for locking?

Right, that's exactly where we want to end up.  This intermediate state of
the patch is just an attempt to verify that we understand when and how
relying on rellockmode will change the behavior.

> Also, are the discrepancies like this to be
> considered bugs of the existing logic?

Mmm ... hard to say.  In the DO ALSO case, it's possible that query
execution would take AccessShareLock and then RowExclusiveLock, which
at least in principle creates a lock-upgrade deadlock hazard.  So I
think standardizing on taking the rellockmode will be an improvement,
but I don't know that I'd call the existing behavior a bug; I certainly
wouldn't risk trying to back-patch a change for it.

In the ROW_MARK_COPY case, it's conceivable that with the existing code
query re-execution would take only AccessShareLock on a FOR UPDATE target
table, where the parser had originally taken RowShareLock.  Again, it
seems like making the behavior more consistent is an improvement, but
not something we'd try to back-patch.

			regards, tom lane

  29. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T13:49:44Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/01 2:18, Tom Lane wrote:
>> I think that the call sites should ultimately look like
>> Assert(CheckRelationLockedByMe(...));
>> but for hunting down the places where the assertion currently fails,
>> it's more convenient if it's just an elog(WARNING).

> Should this check that we're not in a parallel worker process?

Hmm.  I've not seen any failures in the parallel parts of the regular
regression tests, but maybe I'd better do a force_parallel_mode
run before committing.

In general, I'm not on board with the idea that parallel workers don't
need to get their own locks, so I don't really want to exclude parallel
workers from this check.  But if it's not safe for that today, fixing it
is beyond the scope of this particular patch.

			regards, tom lane

  30. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-10-01T19:04:22Z

    On 1 October 2018 at 19:39, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
> etc.), I wonder whether we couldn't just *not* recalculate the lock mode
> based on inspecting the query tree to cross-check with rellockmode?  Why
> not just use rellockmode for locking?  Maybe, okay to keep doing that in
> debug builds though.  Also, are the discrepancies like this to be
> considered bugs of the existing logic?

I got the impression Tom was just leaving that in for a while to let
the buildfarm verify the new code is getting the same lock level as
the old code. Of course, I might be wrong.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  31. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-01T21:25:40Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> On 1 October 2018 at 19:39, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
>> etc.), I wonder whether we couldn't just *not* recalculate the lock mode
>> based on inspecting the query tree to cross-check with rellockmode?  Why
>> not just use rellockmode for locking?  Maybe, okay to keep doing that in
>> debug builds though.  Also, are the discrepancies like this to be
>> considered bugs of the existing logic?

> I got the impression Tom was just leaving that in for a while to let
> the buildfarm verify the new code is getting the same lock level as
> the old code. Of course, I might be wrong.

Yeah, exactly.  My plan is to next switch to taking the locks based on
rellockmode, and then if that doesn't show any problems to switch the
executor to just Assert that there's already a suitable lock, and then
lastly to proceed with ripping out the no-longer-needed logic that
supports the downstream calculations of lockmode.  So a bit more granular
than what Amit submitted, but we'll get to the same place in the end,
with more confidence that we didn't break anything.

			regards, tom lane

  32. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-03T20:16:11Z

    I wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> Should this check that we're not in a parallel worker process?

> Hmm.  I've not seen any failures in the parallel parts of the regular
> regression tests, but maybe I'd better do a force_parallel_mode
> run before committing.
> In general, I'm not on board with the idea that parallel workers don't
> need to get their own locks, so I don't really want to exclude parallel
> workers from this check.  But if it's not safe for that today, fixing it
> is beyond the scope of this particular patch.

So the place where that came out in the wash is the commit I just made
(9a3cebeaa) to change the executor from taking table locks to asserting
that somebody else took them already.  To make that work, I had to make
both ExecOpenScanRelation and relation_open skip checking for lock-held
if IsParallelWorker().

This makes me entirely uncomfortable with the idea that parallel workers
can be allowed to not take any locks of their own.  There is no basis
for arguing that we have field proof that that's safe, because *up to
now, parallel workers in fact did take their own locks*.  And it seems
unsafe on its face, because there's nothing that really guarantees that
the parent process won't go away while children are still running.
(elog(FATAL) seems like a counterexample, for instance.)

I think that we ought to adjust parallel query to insist that children
do take locks, and then revert the IsParallelWorker() exceptions I made
here.  I plan to leave that point in abeyance till we've got the rest
of these changes in place, though.  The easiest way to do it will
doubtless change once we've centralized the executor's table-opening
logic, so trying to code it right now seems like a waste of effort.

			regards, tom lane

  33. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-04T06:59:36Z

    On 2018/10/04 5:16, Tom Lane wrote:
> I wrote:
>> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>>> Should this check that we're not in a parallel worker process?
> 
>> Hmm.  I've not seen any failures in the parallel parts of the regular
>> regression tests, but maybe I'd better do a force_parallel_mode
>> run before committing.
>> In general, I'm not on board with the idea that parallel workers don't
>> need to get their own locks, so I don't really want to exclude parallel
>> workers from this check.  But if it's not safe for that today, fixing it
>> is beyond the scope of this particular patch.
> 
> So the place where that came out in the wash is the commit I just made
> (9a3cebeaa) to change the executor from taking table locks to asserting
> that somebody else took them already.

Thanks for getting that done.

> To make that work, I had to make
> both ExecOpenScanRelation and relation_open skip checking for lock-held
> if IsParallelWorker().

Yeah, I had to do that to when rebasing the remaining patches.

> This makes me entirely uncomfortable with the idea that parallel workers
> can be allowed to not take any locks of their own.  There is no basis
> for arguing that we have field proof that that's safe, because *up to
> now, parallel workers in fact did take their own locks*.  And it seems
> unsafe on its face, because there's nothing that really guarantees that
> the parent process won't go away while children are still running.
> (elog(FATAL) seems like a counterexample, for instance.)
> 
> I think that we ought to adjust parallel query to insist that children
> do take locks, and then revert the IsParallelWorker() exceptions I made
> here.

Maybe I'm missing something here, but isn't the necessary adjustment just
that the relations are opened with locks if inside a parallel worker?

>  I plan to leave that point in abeyance till we've got the rest
> of these changes in place, though.  The easiest way to do it will
> doubtless change once we've centralized the executor's table-opening
> logic, so trying to code it right now seems like a waste of effort.

Okay.

I've rebased the remaining patches.  I broke down one of the patches into
2 and re-ordered the patches as follows:

0001: introduces a function that opens range table relations and maintains
them in an array indexes by RT index

0002: introduces a new field in EState that's an array of RangeTblEntry
pointers and revises macros used in the executor that access RTEs to
return them from the array (David Rowley co-authored this one)

0003: moves result relation and ExecRowMark initialization out of InitPlan
and into ExecInit* routines of respective nodes

0004: removes useless fields from certain planner nodes whose only purpose
has been to assist the executor lock relations in proper order

0005: teaches planner to remove PlanRowMarks corresponding to dummy relations

Thanks,
Amit

  34. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-04T14:19:24Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/04 5:16, Tom Lane wrote:
>> I think that we ought to adjust parallel query to insist that children
>> do take locks, and then revert the IsParallelWorker() exceptions I made
>> here.

> Maybe I'm missing something here, but isn't the necessary adjustment just
> that the relations are opened with locks if inside a parallel worker?

Yeah, that's one plausible way to fix it.  I hadn't wanted to prejudge
the best way before we finish the other changes, though.

> I've rebased the remaining patches.  I broke down one of the patches into
> 2 and re-ordered the patches as follows:

Thanks, will start looking at these today.

			regards, tom lane

  35. Re: executor relation handling

    Andres Freund <andres@anarazel.de> — 2018-10-04T19:12:27Z

    Hi,

On 2018-10-03 16:16:11 -0400, Tom Lane wrote:
> I wrote:
> > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> >> Should this check that we're not in a parallel worker process?
> 
> > Hmm.  I've not seen any failures in the parallel parts of the regular
> > regression tests, but maybe I'd better do a force_parallel_mode
> > run before committing.
> > In general, I'm not on board with the idea that parallel workers don't
> > need to get their own locks, so I don't really want to exclude parallel
> > workers from this check.  But if it's not safe for that today, fixing it
> > is beyond the scope of this particular patch.
> 
> So the place where that came out in the wash is the commit I just made
> (9a3cebeaa) to change the executor from taking table locks to asserting
> that somebody else took them already.  To make that work, I had to make
> both ExecOpenScanRelation and relation_open skip checking for lock-held
> if IsParallelWorker().
> 
> This makes me entirely uncomfortable with the idea that parallel workers
> can be allowed to not take any locks of their own.  There is no basis
> for arguing that we have field proof that that's safe, because *up to
> now, parallel workers in fact did take their own locks*.  And it seems
> unsafe on its face, because there's nothing that really guarantees that
> the parent process won't go away while children are still running.
> (elog(FATAL) seems like a counterexample, for instance.)

> I think that we ought to adjust parallel query to insist that children
> do take locks, and then revert the IsParallelWorker() exceptions I made
> here.  I plan to leave that point in abeyance till we've got the rest
> of these changes in place, though.  The easiest way to do it will
> doubtless change once we've centralized the executor's table-opening
> logic, so trying to code it right now seems like a waste of effort.

I've not really followed this thread, and just caught up to here.  It
seems entirely unacceptable to not acquire locks on workers to me.
Maybe I'm missing something, but why do/did the patches in this thread
require that / introduce that? We didn't have that kind of concept
before, no?  The group locking stuff should rely / require that kind of
thing, no?

Greetings,

Andres Freund

  36. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-04T19:27:59Z

    Andres Freund <andres@anarazel.de> writes:
> I've not really followed this thread, and just caught up to here.  It
> seems entirely unacceptable to not acquire locks on workers to me.
> Maybe I'm missing something, but why do/did the patches in this thread
> require that / introduce that? We didn't have that kind of concept
> before, no?  The group locking stuff should rely / require that kind of
> thing, no?

I'm possibly confused, but I thought that the design of parallel query
involved an expectation that workers didn't need to get their own locks.
What we've determined so far in this thread is that workers *do* get
their own locks (or did before yesterday), but I'd been supposing that
that was accidental not intentional.

In any case, I definitely intend that they will be getting their own
locks again after the dust has settled.  Panic not.

			regards, tom lane

  37. Re: executor relation handling

    Andres Freund <andres@anarazel.de> — 2018-10-04T19:34:44Z

    Hi,

On 2018-10-04 15:27:59 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I've not really followed this thread, and just caught up to here.  It
> > seems entirely unacceptable to not acquire locks on workers to me.
> > Maybe I'm missing something, but why do/did the patches in this thread
> > require that / introduce that? We didn't have that kind of concept
> > before, no?  The group locking stuff should rely / require that kind of
> > thing, no?
> 
> I'm possibly confused, but I thought that the design of parallel query
> involved an expectation that workers didn't need to get their own
> locks.

Not as far as I'm aware of - but I'm not exactly the expert
there. There's an exception that some lock classes don't conflict
between the leader and the workers - that's group locking
(a1c1af2a1f60). But the locks still have to be acquired, and I think
it's quite dangerous not to do so.  The group locking logic is required
because otherwise it'd be trivial to get into deadlocks, and some of the
restrictions around parallel query are required to make that safe.


> What we've determined so far in this thread is that workers *do* get
> their own locks (or did before yesterday), but I'd been supposing that
> that was accidental not intentional.

I don't think it was accidental.

Greetings,

Andres Freund

  38. Re: executor relation handling

    Andres Freund <andres@anarazel.de> — 2018-10-04T19:56:09Z

    On 2018-10-04 12:34:44 -0700, Andres Freund wrote:
> Hi,
>
> On 2018-10-04 15:27:59 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > I've not really followed this thread, and just caught up to here.  It
> > > seems entirely unacceptable to not acquire locks on workers to me.
> > > Maybe I'm missing something, but why do/did the patches in this thread
> > > require that / introduce that? We didn't have that kind of concept
> > > before, no?  The group locking stuff should rely / require that kind of
> > > thing, no?
> >
> > I'm possibly confused, but I thought that the design of parallel query
> > involved an expectation that workers didn't need to get their own
> > locks.
>
> Not as far as I'm aware of - but I'm not exactly the expert
> there. There's an exception that some lock classes don't conflict
> between the leader and the workers - that's group locking
> (a1c1af2a1f60). But the locks still have to be acquired, and I think
> it's quite dangerous not to do so.  The group locking logic is required
> because otherwise it'd be trivial to get into deadlocks, and some of the
> restrictions around parallel query are required to make that safe.

Re-read docs + code just to make sure.  Here's the relevant readme parts:

src/backend/access/transam/README.parallel

To prevent unprincipled deadlocks when running in parallel mode, this code
also arranges for the leader and all workers to participate in group
locking.  See src/backend/storage/lmgr/README for more details.


src/backend/storage/lmgr/README:

Group Locking
-------------

As if all of that weren't already complicated enough, PostgreSQL now supports
parallelism (see src/backend/access/transam/README.parallel), which means that
we might need to resolve deadlocks that occur between gangs of related
processes rather than individual processes.  This doesn't change the basic
deadlock detection algorithm very much, but it makes the bookkeeping more
complicated.

We choose to regard locks held by processes in the same parallel group as
non-conflicting.  This means that two processes in a parallel group can hold a
self-exclusive lock on the same relation at the same time, or one process can
acquire an AccessShareLock while the other already holds AccessExclusiveLock.
This might seem dangerous and could be in some cases (more on that below), but
if we didn't do this then parallel query would be extremely prone to
self-deadlock.  For example, a parallel query against a relation on which the
leader already had AccessExclusiveLock would hang, because the workers would
try to lock the same relation and be blocked by the leader; yet the leader
can't finish until it receives completion indications from all workers.  An
undetected deadlock results.  This is far from the only scenario where such a
problem happens.  The same thing will occur if the leader holds only
AccessShareLock, the worker seeks AccessShareLock, but between the time the
leader attempts to acquire the lock and the time the worker attempts to
acquire it, some other process queues up waiting for an AccessExclusiveLock.
In this case, too, an indefinite hang results.

...


So yes, locks are expected to be acquired in workers.

Greetings,

Andres Freund

  39. Re: executor relation handling

    Robert Haas <robertmhaas@gmail.com> — 2018-10-04T19:57:40Z

    On Thu, Oct 4, 2018 at 3:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm possibly confused, but I thought that the design of parallel query
> involved an expectation that workers didn't need to get their own locks.

You are, indeed, confused.  A heck of a lot of effort went into making
sure that the workers COULD take their own locks, and into trying to
make sure that didn't break anything.  That effort may or may not have
been entirely successful, but I'm pretty sure that having them NOT
take locks is going to be a lot worse.

> What we've determined so far in this thread is that workers *do* get
> their own locks (or did before yesterday), but I'd been supposing that
> that was accidental not intentional.

Nope, that was intentional.

> In any case, I definitely intend that they will be getting their own
> locks again after the dust has settled.  Panic not.

/me unloads metaphorical bazooka.

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

  40. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-04T20:40:25Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Oct 4, 2018 at 3:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What we've determined so far in this thread is that workers *do* get
>> their own locks (or did before yesterday), but I'd been supposing that
>> that was accidental not intentional.

> Nope, that was intentional.

Fair enough --- in which case, the patch series we're working on here
was broken to suppose that it could get away with removing that.

As I said, I'll make sure the locking is back before I finish with this.

			regards, tom lane

  41. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-04T20:59:57Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> I've rebased the remaining patches.  I broke down one of the patches into
> 2 and re-ordered the patches as follows:

> 0001: introduces a function that opens range table relations and maintains
> them in an array indexes by RT index

> 0002: introduces a new field in EState that's an array of RangeTblEntry
> pointers and revises macros used in the executor that access RTEs to
> return them from the array (David Rowley co-authored this one)

I've pushed 0001 and 0002 with mostly cosmetic changes.  One thing I
wanted to point out explicitly, though, is that I found this bit of 0002
to be a seriously bad idea:

--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -20,6 +20,7 @@
 #include "executor/instrument.h"
 #include "lib/pairingheap.h"
 #include "nodes/params.h"
+#include "nodes/parsenodes.h"
 #include "nodes/plannodes.h"
 #include "utils/hsearch.h"
 #include "utils/queryenvironment.h"

Please do not add #includes of fundamental headers to other fundamental
headers without clearing it with somebody.  There's little enough
structure to our header collection now.  I don't want to end up in a
situation where effectively the entire header set gets pulled into
every .c file, or worse that we have actual reference loops in the
headers.  (This is not an academic problem; somebody actually created
such a loop awhile back.  Cleaning it up, by the time we'd recognized
the problem, was really painful.)

> 0003: moves result relation and ExecRowMark initialization out of InitPlan
> and into ExecInit* routines of respective nodes

I am finding myself pretty unconvinced by this one; it seems like mostly
a random reallocation of responsibility with little advantage.  The
particular thing that brought me to a screeching halt was seeing that
the patch removed ExecFindRowMark, despite the fact that that's part
of our advertised FDW API (see fdwhandler.sgml), and it didn't provide
any alternative way for an FDW to find out at runtime whether it's
subject to a row locking requirement.

I thought for a minute about just leaving the function in place, but
that wouldn't work because both nodeLockRows and nodeModifyTable are
written so that they find^H^H^Hbuild their rowmarks only after recursing
to initialize their child plan nodes; so a child node that tried to use
ExecFindRowMark during ExecInitNode would get the wrong answer.  Of
course, we could consider changing the order of operations during
initialization of those node types, but I'm not really seeing a compelling
reason why we should whack things around that much.

So I'm inclined to just omit 0003.  AFAICS this would only mean that
we couldn't drop the global PlanRowMarks list from PlannedStmt, which
does not bother me much.

> 0005: teaches planner to remove PlanRowMarks corresponding to dummy relations

I'm not entirely sold on the value of that either?

			regards, tom lane

  42. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-05T02:06:29Z

    On 2018/10/05 5:59, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> I've rebased the remaining patches.  I broke down one of the patches into
>> 2 and re-ordered the patches as follows:
> 
>> 0001: introduces a function that opens range table relations and maintains
>> them in an array indexes by RT index
> 
>> 0002: introduces a new field in EState that's an array of RangeTblEntry
>> pointers and revises macros used in the executor that access RTEs to
>> return them from the array (David Rowley co-authored this one)
> 
> I've pushed 0001 and 0002 with mostly cosmetic changes.

Thanks a lot.

> One thing I
> wanted to point out explicitly, though, is that I found this bit of 0002
> to be a seriously bad idea:
> 
> --- a/src/include/nodes/execnodes.h
> +++ b/src/include/nodes/execnodes.h
> @@ -20,6 +20,7 @@
>  #include "executor/instrument.h"
>  #include "lib/pairingheap.h"
>  #include "nodes/params.h"
> +#include "nodes/parsenodes.h"
>  #include "nodes/plannodes.h"
>  #include "utils/hsearch.h"
>  #include "utils/queryenvironment.h"
> 
> Please do not add #includes of fundamental headers to other fundamental
> headers without clearing it with somebody.  There's little enough
> structure to our header collection now.  I don't want to end up in a
> situation where effectively the entire header set gets pulled into
> every .c file, or worse that we have actual reference loops in the
> headers.  (This is not an academic problem; somebody actually created
> such a loop awhile back.  Cleaning it up, by the time we'd recognized
> the problem, was really painful.)

Okay, sorry about that.  I was slightly nervous that I had to do it when
doing it, but forgot to mention that explicitly in the commit message or
the email.

>> 0003: moves result relation and ExecRowMark initialization out of InitPlan
>> and into ExecInit* routines of respective nodes
> 
> I am finding myself pretty unconvinced by this one; it seems like mostly
> a random reallocation of responsibility with little advantage.  The
> particular thing that brought me to a screeching halt was seeing that
> the patch removed ExecFindRowMark, despite the fact that that's part
> of our advertised FDW API (see fdwhandler.sgml), and it didn't provide
> any alternative way for an FDW to find out at runtime whether it's
> subject to a row locking requirement.
> 
> I thought for a minute about just leaving the function in place, but
> that wouldn't work because both nodeLockRows and nodeModifyTable are
> written so that they find^H^H^Hbuild their rowmarks only after recursing
> to initialize their child plan nodes; so a child node that tried to use
> ExecFindRowMark during ExecInitNode would get the wrong answer.  Of
> course, we could consider changing the order of operations during
> initialization of those node types, but I'm not really seeing a compelling
> reason why we should whack things around that much.
> 
> So I'm inclined to just omit 0003.  AFAICS this would only mean that
> we couldn't drop the global PlanRowMarks list from PlannedStmt, which
> does not bother me much.

To be honest, I too had begun to fail to see the point of this patch since
yesterday.  In fact, getting this one to pass make check-world took a bit
of head-scratching due to its interaction with EvalPlanQuals EState
building, so I was almost to drop it from the series.  But I felt that it
might still be a good idea to get rid of what was described as special
case code.  Reading your argument against it based on how it messes up
some of the assumptions regarding ExecRowMark design, I'm willing to let
go of this one as more or less a cosmetic improvement.

>> 0005: teaches planner to remove PlanRowMarks corresponding to dummy relations
> 
> I'm not entirely sold on the value of that either?

If you look at the first email on this thread, you can see that trimming
the PlanRowMarks list down to just the ones needed during execution
results in non-trivial improvement in SELECT FOR SHARE on partitioned
tables with large number of partitions:

<quote>
Speedup is more pronounced with a benchmark that needs RowMarks, because
one of the patches (0003) removes overhead around handling them.

$ cat /tmp/select-lt-for-share.sql
select * from lt where b = 999 for share;

master

tps = 94.095985 (excluding connections establishing)
tps = 93.955702 (excluding connections establishing)

<snip>

patch 0003 (prune PlanRowMarks)

tps = 712.544029 (excluding connections establishing)
tps = 717.540052 (excluding connections establishing)
</quote>

But on reflection, this seems more like adding special case code to the
planner just for this, rather than solving the more general problem of
initializing *any* partition information after pruning, being discussed
over at "speeding up planning with partitions" thread [1].  So, I don't
mind dropping this one too.


So, that leaves us with only the patch that revises the plan node fields
in light of having relieved the executor of doing any locking of its own
in *some* cases.  That patch's premise is that we don't need the fields
that the patch removes because they're only referenced for the locking
purposes.  But, if those plan nodes support parallel execution and hence
will be passed to parallel workers for execution who will need to lock the
tables contained in the plan nodes, then they better contain all the
information needed for locking *every* affected tables, so we had better
not removed it.  Plan nodes in question are Append, MergeAppend, and
ModifyTable, of which only the first two support parallel execution, but
maybe we should just leave all of them alone for now.  IOW, I'm not sure
about that patch either.  Thoughts?

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/9d7c5112-cb99-6a47-d3be-cf1ee6862a1d@lab.ntt.co.jp

  43. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-06T18:59:08Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/05 5:59, Tom Lane wrote:
>> So I'm inclined to just omit 0003.  AFAICS this would only mean that
>> we couldn't drop the global PlanRowMarks list from PlannedStmt, which
>> does not bother me much.

> To be honest, I too had begun to fail to see the point of this patch since
> yesterday.  In fact, getting this one to pass make check-world took a bit
> of head-scratching due to its interaction with EvalPlanQuals EState
> building, so I was almost to drop it from the series.  But I felt that it
> might still be a good idea to get rid of what was described as special
> case code.  Reading your argument against it based on how it messes up
> some of the assumptions regarding ExecRowMark design, I'm willing to let
> go of this one as more or less a cosmetic improvement.

OK.  We do need to fix the comments in InitPlan that talk about acquiring
locks and how that makes us need to do things in a particular order, but
I'll go take care of that.

> So, that leaves us with only the patch that revises the plan node fields
> in light of having relieved the executor of doing any locking of its own
> in *some* cases.  That patch's premise is that we don't need the fields
> that the patch removes because they're only referenced for the locking
> purposes.  But, if those plan nodes support parallel execution and hence
> will be passed to parallel workers for execution who will need to lock the
> tables contained in the plan nodes, then they better contain all the
> information needed for locking *every* affected tables, so we had better
> not removed it.

No, I think this is unduly pessimistic.  We need to make sure that a
parallel worker has lock on tables it's actually touching, but I don't
see why that should imply a requirement to hold lock on parent tables
it never touches.

The reasons why we need locks on tables not physically accessed by the
query are (a) to ensure that we've blocked, or received sinval messages
for, any DDL related to views or partition parent tables, in case that
would invalidate the plan; (b) to allow firing triggers safely, in
the case of partition parent tables.  Neither of these issues apply to
a parallel worker -- the plan is already frozen before it can ever
start, and it isn't going to be firing any triggers either.

In particular, I think it's fine to get rid of
ExecLockNonLeafAppendTables.  In the parent, that's clearly useless code
now: we have already locked *every* RTE_RELATION entry in the rangetable,
either when the parser/rewriter/planner added the RTE to the list to begin
with, or during AcquireExecutorLocks if the plan was retrieved from the
plancache.  In a child it seems unnecessary as long as we're locking the
leaf rels we actually touch.

Possibly, we might fix the problem of inadequate locking in worker
processes by having them do the equivalent of AcquireExecutorLocks, ie
just run through the whole RT list and lock everything.  I think we'd soon
end up doing that if we ever try to allow parallelization of non-read-only
queries; but that's a long way off AFAIK.  For read-only queries it seems
like it'll be fine if we just rejigger ExecGetRangeTableRelation to take a
lock when IsParallelWorker.

			regards, tom lane

  44. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-07T18:55:07Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> 0004: removes useless fields from certain planner nodes whose only purpose
> has been to assist the executor lock relations in proper order

I've pushed most of 0004 now; obviously, not the parts removing
PlannedStmt.rowMarks, since that's not possible without rearrangement
of the executor's RowMark handling.

I didn't like the idea of unifying ModifyTable.nominalRelation with
the partition root info.  Those fields serve different masters ---
nominalRelation, at least in its original intent, is only meant for
use of EXPLAIN and might have nothing to do with what happens at
execution.  So even though unifying them would work today, we might
regret it down the line.  Instead I left that field alone and added
a separate rootRelation field to carry the partition root RT index,
which ends up being the same number of fields anyway since we don't
need a flag for is-the-nominal-relation-a-partition-root.

Still need to think a bit more about whether we want 0005 in
anything like its current form.

			regards, tom lane

  45. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-07T23:18:15Z

    I wrote:
> Still need to think a bit more about whether we want 0005 in
> anything like its current form.

So I poked at that for a bit, and soon realized that the *main* problem
there is that ExecFindRowMark() eats O(N^2) time due to repeated searches
of the es_rowMarks list.  While the patch as stated would improve that
for cases where most of the partitions can be pruned at plan time, it
does nothing much for cases where they can't.  However, it's pretty
trivial to fix that: let's just use an array not a list.  Patch 0001
attached does that.

A further improvement we could consider is to avoid opening the relcache
entries for pruned-away relations.  I could not find a way to do that
that was less invasive than removing ExecRowMark.relation and requiring
callers to call a new function to get the relation if they need it.
Patch 0002 attached is a delta on top of 0001 that does that.

Replicating your select-lt-for-share test case as best I can (you never
actually specified it carefully), I find that the TPS rate on my
workstation goes from about 250 tps with HEAD to 920 tps with patch 0001
or 1130 tps with patch 0002.  This compares to about 1600 tps for the
non-FOR-SHARE version of the query.

However, we should keep in mind that without partitioning overhead
(ie "select * from lt_999 where b = 999 for share"), the TPS rate
is over 25800 tps.  Most of the overhead in the partitioned case seems
to be from acquiring locks on rangetable entries that we won't ever
use, and none of these patch variants are touching that problem.
So ISTM that the *real* win for this scenario is going to come from
teaching the system to prune unwanted relations from the query
altogether, not just from the PlanRowMark list.

Keeping that comparison in mind, I'm inclined to think that 0001
is the best thing to do for now.  The incremental win from 0002
is not big enough to justify the API break it creates, while your
0005 is not really attacking the problem the right way.

			regards, tom lane

  46. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-10-07T23:55:35Z

    On 8 October 2018 at 12:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> However, we should keep in mind that without partitioning overhead
> (ie "select * from lt_999 where b = 999 for share"), the TPS rate
> is over 25800 tps.  Most of the overhead in the partitioned case seems
> to be from acquiring locks on rangetable entries that we won't ever
> use, and none of these patch variants are touching that problem.
> So ISTM that the *real* win for this scenario is going to come from
> teaching the system to prune unwanted relations from the query
> altogether, not just from the PlanRowMark list.

Idle thought:  I wonder if we could add another field to the
RangeTblEntry; "delaylock". Set that to true in the planner for all
other_member rels that are partitions then not obtain locks on those
during AcquireExecutorLocks(). Instead, grab the lock in
ExecGetRangeTableRelation() the first time through.

We'd still obtain the lock for the table named in the query at the
normal time so cached plans could properly be invalidated. We'd need
to ensure that anything that could be changed in the partitions to
cause a plan to become invalid properly obtains a lock on the
partitioned table, all the way to the top of the hierarchy.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  47. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-08T00:13:54Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> On 8 October 2018 at 12:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So ISTM that the *real* win for this scenario is going to come from
>> teaching the system to prune unwanted relations from the query
>> altogether, not just from the PlanRowMark list.

> Idle thought:  I wonder if we could add another field to the
> RangeTblEntry; "delaylock". Set that to true in the planner for all
> other_member rels that are partitions then not obtain locks on those
> during AcquireExecutorLocks(). Instead, grab the lock in
> ExecGetRangeTableRelation() the first time through.

Hmm, I'm afraid that's not terribly safe, unless there are ALTER
TABLE restrictions on partitions that I'm not aware of.  For instance,
a leaf partition might have an index it didn't inherit from the parent,
and the query plan might be intending to use that index.  If you don't
take lock on the leaf, you might not know that the index has been dropped
so that a new plan is needed.

The idea I had in mind was to allow hard pruning of any leaf that's
been excluded *at plan time* based on partition constraints seen in
its parent rel(s).  That should be safe enough as long as we take
locks on all the non-leaf rels --- then we'd know about any change
in the constraint situation.

Rather than coping with renumbering the RTEs, it might be easiest
to invent an "RTE_DUMMY" RTE type that a hard-pruned RTE could be
changed to.

			regards, tom lane

  48. Re: executor relation handling

    David Rowley <david.rowley@2ndquadrant.com> — 2018-10-08T00:29:48Z

    On 8 October 2018 at 13:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The idea I had in mind was to allow hard pruning of any leaf that's
> been excluded *at plan time* based on partition constraints seen in
> its parent rel(s).  That should be safe enough as long as we take
> locks on all the non-leaf rels --- then we'd know about any change
> in the constraint situation.
>
> Rather than coping with renumbering the RTEs, it might be easiest
> to invent an "RTE_DUMMY" RTE type that a hard-pruned RTE could be
> changed to.

The problem with that is that, if we get [1] done in PG12, then the
RTE_DUMMYs would not exist, as we'd only have RTEs in the range table
for partitions that survived plan-time pruning.

It also leaves a problem to solve in the unneeded locks being taken on
partitions for PREPAREd queries using run-time pruning.

[1] https://commitfest.postgresql.org/20/1778/

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  49. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-08T15:38:44Z

    I wrote:
> Keeping that comparison in mind, I'm inclined to think that 0001
> is the best thing to do for now.  The incremental win from 0002
> is not big enough to justify the API break it creates, while your
> 0005 is not really attacking the problem the right way.

I've pushed 0001 now.  I believe that closes out all the patches
discussed in this thread, so I've marked the CF entry committed.
Thanks for all the hard work!

			regards, tom lane

  50. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-09T06:11:38Z

    On 2018/10/07 3:59, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> On 2018/10/05 5:59, Tom Lane wrote:
>>> So I'm inclined to just omit 0003.  AFAICS this would only mean that
>>> we couldn't drop the global PlanRowMarks list from PlannedStmt, which
>>> does not bother me much.
> 
>> To be honest, I too had begun to fail to see the point of this patch since
>> yesterday.  In fact, getting this one to pass make check-world took a bit
>> of head-scratching due to its interaction with EvalPlanQuals EState
>> building, so I was almost to drop it from the series.  But I felt that it
>> might still be a good idea to get rid of what was described as special
>> case code.  Reading your argument against it based on how it messes up
>> some of the assumptions regarding ExecRowMark design, I'm willing to let
>> go of this one as more or less a cosmetic improvement.
> 
> OK.  We do need to fix the comments in InitPlan that talk about acquiring
> locks and how that makes us need to do things in a particular order, but
> I'll go take care of that.

Thanks for doing that.

>> So, that leaves us with only the patch that revises the plan node fields
>> in light of having relieved the executor of doing any locking of its own
>> in *some* cases.  That patch's premise is that we don't need the fields
>> that the patch removes because they're only referenced for the locking
>> purposes.  But, if those plan nodes support parallel execution and hence
>> will be passed to parallel workers for execution who will need to lock the
>> tables contained in the plan nodes, then they better contain all the
>> information needed for locking *every* affected tables, so we had better
>> not removed it.
> 
> No, I think this is unduly pessimistic.  We need to make sure that a
> parallel worker has lock on tables it's actually touching, but I don't
> see why that should imply a requirement to hold lock on parent tables
> it never touches.
> 
> The reasons why we need locks on tables not physically accessed by the
> query are (a) to ensure that we've blocked, or received sinval messages
> for, any DDL related to views or partition parent tables, in case that
> would invalidate the plan; (b) to allow firing triggers safely, in
> the case of partition parent tables.  Neither of these issues apply to
> a parallel worker -- the plan is already frozen before it can ever
> start, and it isn't going to be firing any triggers either.
> 
> In particular, I think it's fine to get rid of
> ExecLockNonLeafAppendTables.  In the parent, that's clearly useless code
> now: we have already locked *every* RTE_RELATION entry in the rangetable,
> either when the parser/rewriter/planner added the RTE to the list to begin
> with, or during AcquireExecutorLocks if the plan was retrieved from the
> plancache.  In a child it seems unnecessary as long as we're locking the
> leaf rels we actually touch.
> 
> Possibly, we might fix the problem of inadequate locking in worker
> processes by having them do the equivalent of AcquireExecutorLocks, ie
> just run through the whole RT list and lock everything.  I think we'd soon
> end up doing that if we ever try to allow parallelization of non-read-only
> queries; but that's a long way off AFAIK.  For read-only queries it seems
> like it'll be fine if we just rejigger ExecGetRangeTableRelation to take a
> lock when IsParallelWorker.

Okay, thanks for the explanation.  It's now clearer to me why parallel
workers don't really need to lock non-leaf relations.

Thanks,
Amit

  51. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-09T06:26:34Z

    On 2018/10/08 3:55, Tom Lane wrote:
> I didn't like the idea of unifying ModifyTable.nominalRelation with
> the partition root info.  Those fields serve different masters ---
> nominalRelation, at least in its original intent, is only meant for
> use of EXPLAIN and might have nothing to do with what happens at
> execution.  So even though unifying them would work today, we might
> regret it down the line.  Instead I left that field alone and added
> a separate rootRelation field to carry the partition root RT index,
> which ends up being the same number of fields anyway since we don't
> need a flag for is-the-nominal-relation-a-partition-root.

Thanks for pushing that.  I'd also named it 'rootRelation' in my original
patch before David had objected to calling it that, because a command may
not specify the "actual" root of a partition tree; it could be a non-root
partitioned table.  He'd suggested 'partitionedTarget' for the new field
[1], stressing the "target" part.  Maybe, 'rootRelation' isn't too
confusing though.

Thanks,a
Amit

[1]
https://www.postgresql.org/message-id/CAKJS1f_M0jkgL-d%3Dk-rf6TMzghATDmZ67nzja1tz4h3G%3D27e7Q%40mail.gmail.com

  52. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-09T07:17:20Z

    On 2018/10/09 0:38, Tom Lane wrote:
> I wrote:
>> Keeping that comparison in mind, I'm inclined to think that 0001
>> is the best thing to do for now.  The incremental win from 0002
>> is not big enough to justify the API break it creates, while your
>> 0005 is not really attacking the problem the right way.
> 
> I've pushed 0001 now.  I believe that closes out all the patches
> discussed in this thread, so I've marked the CF entry committed.
> Thanks for all the hard work!

Thanks a lot for reviewing and committing.

Regards,
Amit

  53. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-09T07:31:06Z

    On 2018/10/08 9:29, David Rowley wrote:
> On 8 October 2018 at 13:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The idea I had in mind was to allow hard pruning of any leaf that's
>> been excluded *at plan time* based on partition constraints seen in
>> its parent rel(s).  That should be safe enough as long as we take
>> locks on all the non-leaf rels --- then we'd know about any change
>> in the constraint situation.
>>
>> Rather than coping with renumbering the RTEs, it might be easiest
>> to invent an "RTE_DUMMY" RTE type that a hard-pruned RTE could be
>> changed to.
> 
> The problem with that is that, if we get [1] done in PG12, then the
> RTE_DUMMYs would not exist, as we'd only have RTEs in the range table
> for partitions that survived plan-time pruning.
...

> [1] https://commitfest.postgresql.org/20/1778/

Yeah, the patch proposed at [1] postpones creating partition RTEs (hence
locking them) to a point after pruning, which also means we create only
the necessary RTEs.  In fact, it's not just the RTEs, but child
PlanRowMarks, whose creation is postponed to after pruning.  So, I
admitted upthread that my proposed patch here would only add code that
will become useless if we're able to get [1] in.

Thanks,
Amit

  54. Re: executor relation handling

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-09T08:00:51Z

    On 2018/10/08 8:18, Tom Lane wrote:
> I wrote:
>> Still need to think a bit more about whether we want 0005 in
>> anything like its current form.
> 
> So I poked at that for a bit, and soon realized that the *main* problem
> there is that ExecFindRowMark() eats O(N^2) time due to repeated searches
> of the es_rowMarks list.

Yeah, I proposed the patch only because of stumbling across O(N^2)
behavior, but went to solve it with the planner hack, which I agree is an
inferior solution overall.

> While the patch as stated would improve that
> for cases where most of the partitions can be pruned at plan time, it
> does nothing much for cases where they can't.  However, it's pretty
> trivial to fix that: let's just use an array not a list.  Patch 0001
> attached does that.
>
> A further improvement we could consider is to avoid opening the relcache
> entries for pruned-away relations.  I could not find a way to do that
> that was less invasive than removing ExecRowMark.relation and requiring
> callers to call a new function to get the relation if they need it.
> Patch 0002 attached is a delta on top of 0001 that does that.
>
> Replicating your select-lt-for-share test case as best I can (you never
> actually specified it carefully), I find that the TPS rate on my
> workstation goes from about 250 tps with HEAD to 920 tps with patch 0001
> or 1130 tps with patch 0002.  This compares to about 1600 tps for the
> non-FOR-SHARE version of the query.
>
> However, we should keep in mind that without partitioning overhead
> (ie "select * from lt_999 where b = 999 for share"), the TPS rate
> is over 25800 tps.  Most of the overhead in the partitioned case seems
> to be from acquiring locks on rangetable entries that we won't ever
> use, and none of these patch variants are touching that problem.
> So ISTM that the *real* win for this scenario is going to come from
> teaching the system to prune unwanted relations from the query
> altogether, not just from the PlanRowMark list.

Agreed, which is why I mentioned the other patch [1], which gets us closer
to that goal.

> Keeping that comparison in mind, I'm inclined to think that 0001
> is the best thing to do for now.  The incremental win from 0002
> is not big enough to justify the API break it creates, while your
> 0005 is not really attacking the problem the right way.

I agree that 0002's improvement is only incremental and would lose its
appeal if we're able to solve the bigger problem of removing range table
and other overhead when planning with large number of partitions, but that
might take a while.

Thanks,
Amit

[1] https://commitfest.postgresql.org/20/1778/

  55. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-09T14:07:41Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/08 3:55, Tom Lane wrote:
>> I didn't like the idea of unifying ModifyTable.nominalRelation with
>> the partition root info.  Those fields serve different masters ---
>> nominalRelation, at least in its original intent, is only meant for
>> use of EXPLAIN and might have nothing to do with what happens at
>> execution.  So even though unifying them would work today, we might
>> regret it down the line.  Instead I left that field alone and added
>> a separate rootRelation field to carry the partition root RT index,
>> which ends up being the same number of fields anyway since we don't
>> need a flag for is-the-nominal-relation-a-partition-root.

> Thanks for pushing that.  I'd also named it 'rootRelation' in my original
> patch before David had objected to calling it that, because a command may
> not specify the "actual" root of a partition tree; it could be a non-root
> partitioned table.  He'd suggested 'partitionedTarget' for the new field
> [1], stressing the "target" part.  Maybe, 'rootRelation' isn't too
> confusing though.

Well, it's the root so far as the current query is concerned --- we do not
take any semantic account of partitioning levels that might exist above
the table named in the query, do we?

			regards, tom lane

  56. Re: executor relation handling

    Amit Langote <amitlangote09@gmail.com> — 2018-10-09T15:16:14Z

    On Tue, Oct 9, 2018 at 11:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> > On 2018/10/08 3:55, Tom Lane wrote:
> >> I didn't like the idea of unifying ModifyTable.nominalRelation with
> >> the partition root info.  Those fields serve different masters ---
> >> nominalRelation, at least in its original intent, is only meant for
> >> use of EXPLAIN and might have nothing to do with what happens at
> >> execution.  So even though unifying them would work today, we might
> >> regret it down the line.  Instead I left that field alone and added
> >> a separate rootRelation field to carry the partition root RT index,
> >> which ends up being the same number of fields anyway since we don't
> >> need a flag for is-the-nominal-relation-a-partition-root.
>
> > Thanks for pushing that.  I'd also named it 'rootRelation' in my original
> > patch before David had objected to calling it that, because a command may
> > not specify the "actual" root of a partition tree; it could be a non-root
> > partitioned table.  He'd suggested 'partitionedTarget' for the new field
> > [1], stressing the "target" part.  Maybe, 'rootRelation' isn't too
> > confusing though.
>
> Well, it's the root so far as the current query is concerned --- we do not
> take any semantic account of partitioning levels that might exist above
> the table named in the query, do we?

We don't, and I personally agree with the reasoning behind calling it
rootRelation.

Thanks,
Amit

  57. Re: executor relation handling

    Robert Haas <robertmhaas@gmail.com> — 2018-10-09T18:27:44Z

    On Sat, Oct 6, 2018 at 2:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The reasons why we need locks on tables not physically accessed by the
> query are (a) to ensure that we've blocked, or received sinval messages
> for, any DDL related to views or partition parent tables, in case that
> would invalidate the plan; (b) to allow firing triggers safely, in
> the case of partition parent tables.  Neither of these issues apply to
> a parallel worker -- the plan is already frozen before it can ever
> start, and it isn't going to be firing any triggers either.

That last part could *easily* change in a future release.  We've
already started to allow CTAS with parallel query, and there have
already been multiple people wanting to allow more.  It would be a
shame if we threw up additional obstacles in the way of that...

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

  58. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-09T18:35:02Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Oct 6, 2018 at 2:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The reasons why we need locks on tables not physically accessed by the
>> query are (a) to ensure that we've blocked, or received sinval messages
>> for, any DDL related to views or partition parent tables, in case that
>> would invalidate the plan; (b) to allow firing triggers safely, in
>> the case of partition parent tables.  Neither of these issues apply to
>> a parallel worker -- the plan is already frozen before it can ever
>> start, and it isn't going to be firing any triggers either.

> That last part could *easily* change in a future release.  We've
> already started to allow CTAS with parallel query, and there have
> already been multiple people wanting to allow more.  It would be a
> shame if we threw up additional obstacles in the way of that...

I hardly think that this is the most serious issue in the way of
doing non-read-only things in parallel workers.

In any case, a parallel worker would surely have to open any
relations it is going to fire triggers for.  If it gets the correct
lock when it does that, all is well.  If not, the Assert in
relation_open will complain.

			regards, tom lane

  59. Re: executor relation handling

    Robert Haas <robertmhaas@gmail.com> — 2018-10-11T03:06:39Z

    On Tue, Oct 9, 2018 at 2:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > That last part could *easily* change in a future release.  We've
> > already started to allow CTAS with parallel query, and there have
> > already been multiple people wanting to allow more.  It would be a
> > shame if we threw up additional obstacles in the way of that...
>
> I hardly think that this is the most serious issue in the way of
> doing non-read-only things in parallel workers.

My concern, as I said, is about adding new obstacles.

> In any case, a parallel worker would surely have to open any
> relations it is going to fire triggers for.  If it gets the correct
> lock when it does that, all is well.  If not, the Assert in
> relation_open will complain.

Well, in that case, no issues.

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

  60. Re: executor relation handling

    Julien Rouhaud <rjuju123@gmail.com> — 2019-02-09T23:24:24Z

    Hi,

On Sun, Sep 30, 2018 at 7:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I think that the call sites should ultimately look like
>
>         Assert(CheckRelationLockedByMe(...));
>
> but for hunting down the places where the assertion currently fails,
> it's more convenient if it's just an elog(WARNING).

I just hit one of the asserts (in relation_open()) added in
b04aeb0a053.  Here's a simple reproducer:

DROP TABLE IF EXISTS test;
CREATE TABLE test (id integer primary key);
PREPARE s AS DELETE FROM test WHERE id = 1;
EXECUTE s;
EXECUTE s;


This comes from ExecInitIndexScan() and ExecInitBitmapIndexScan(),
which open the index without lock if the parent table is a target
relation:

    /*
     * Open the index relation.
     *
     * If the parent table is one of the target relations of the query, then
     * InitPlan already opened and write-locked the index, so we can avoid
     * taking another lock here.  Otherwise we need a normal reader's lock.
     */
    relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
    indexstate->iss_RelationDesc = index_open(node->indexid,
                                              relistarget ? NoLock :
AccessShareLock);

And digging into InitPlan() up to ExecInitModifyTable():

        /*
         * If there are indices on the result relation, open them and save
         * descriptors in the result relation info, so that we can add new
         * index entries for the tuples we add/update.  We need not do this
         * for a DELETE, however, since deletion doesn't affect indexes. Also,
         * inside an EvalPlanQual operation, the indexes might be open
         * already, since we share the resultrel state with the original
         * query.
         */
        if (resultRelInfo->ri_RelationDesc->rd_rel->relhasindex &&
            operation != CMD_DELETE &&
            resultRelInfo->ri_IndexRelationDescs == NULL)
            ExecOpenIndices(resultRelInfo,
                            node->onConflictAction != ONCONFLICT_NONE);


So, this is problematic with a cached plan on a DELETE query since the
lock on the target relation's index will never be acquired (the lock
is acquired on the first execution in get_relation_info()).  This
doesn't seem unsafe though, since DROP INDEX [CONCURRENTLY] will still
acquire lock on the index relation or query xid before dropping the
index.

I'm not sure of what's the best way to fix this problem.  I wanted to
modify ExecInitIndexScan() and ExecInitBitmapIndexScan() to acquire
the lock for a DELETE on the target relation, however I don't think
that we have that information at this point.  Maybe just
unconditionally acquire an AccessShareLock on the index in those
functions?

  61. Re: executor relation handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-09T23:31:49Z

    Julien Rouhaud <rjuju123@gmail.com> writes:
> I just hit one of the asserts (in relation_open()) added in
> b04aeb0a053.  Here's a simple reproducer:

Yeah, I think this is the same issue being discussed in

https://www.postgresql.org/message-id/flat/19465.1541636036%40sss.pgh.pa.us

I imagine the patch David recently posted in that thread will fix this,
but can you check, and/or review the patch?

			regards, tom lane

  62. Re: executor relation handling

    Julien Rouhaud <rjuju123@gmail.com> — 2019-02-10T00:00:58Z

    On Sun, Feb 10, 2019 at 12:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > I just hit one of the asserts (in relation_open()) added in
> > b04aeb0a053.  Here's a simple reproducer:
>
> Yeah, I think this is the same issue being discussed in
>
> https://www.postgresql.org/message-id/flat/19465.1541636036%40sss.pgh.pa.us

Ah indeed, sorry I totally missed that thread.

>> I imagine the patch David recently posted in that thread will fix this,
> but can you check, and/or review the patch?

I tried quickly and it does fix my test case.  It's quite late here,
so I'll review the patch tomorrow.
Thanks.