Re: [HACKERS] [postgresql 10 beta3] unrecognized node type: 90

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Noah Misch <noah(at)leadboat(dot)com>, "Adam, Etienne (Nokia-TECH/Issy Les Moulineaux)" <etienne(dot)adam(at)nokia(dot)com>, PostgreSQL Bugs <pgsql-bugs(at)postgresql(dot)org>, "Duquesne, Pierre (Nokia-TECH/Issy Les Moulineaux)" <pierre(dot)duquesne(at)nokia(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] [postgresql 10 beta3] unrecognized node type: 90
Date: 2017-08-29 16:35:43
Message-ID: 9861.1504024543@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Tue, Aug 29, 2017 at 8:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> There's already ExecParallelReinitialize, which could be made to walk
>> the nodes in addition to what it does already, but I don't understand
>> exactly what else needs fixing.

> Sure, but it is not advisable to reset state of all the nodes below
> gather at that place, otherwise, it will be more or less like we are
> forcing rescan of each node. I think there we can reset the shared
> parallel state of parallel-aware nodes (or anything related) and then
> allow rescan to reset the master backend specific state for all nodes
> beneath gather.

Right, the idea is to make this happen separately from the "rescan"
logic. In general, it's a good idea for ExecReScanFoo to do as little
as possible, so that you don't pay if a node is rescanned more than
once before it's asked to do anything, or indeed if no rows are ever
demanded from it at all.

Attached is a WIP patch along this line. It's unfinished because
I've not done the tedium of extending the FDW and CustomScan APIs
to support this new type of per-node operation; but that part seems
straightforward enough. The core code is complete and survived
light testing. I'm pretty happy with the results --- note in
particular how we get rid of some very dubious coding in
ExecReScanIndexScan and ExecReScanIndexOnlyScan.

If you try the test case from a2b70c89c on this patch alone, you'll notice
that instead of sometimes reporting too-small counts during the rescans,
it pretty consistently reports too-large counts. This is because we are
now successfully resetting the shared state for the parallel seqscan, but
we haven't done anything about the leader's HashAgg node deciding that it
can re-use its old hashtable. So on the first scan, the leader typically
scans all or most of the table because of its startup time advantage, and
saves those counts in its hashtable. On later scans, the workers read all
of the table while the leader decides it need do no scanning. So we get
counts that reflect all of the table (from the workers) plus whatever part
of the table the leader read the first time. So this by no means removes
the need for my other patch.

If no objections, I'll do the additional legwork and push. As before,
I think we can probably get away without fixing 9.6, even though it's
nominally got the same bug.

regards, tom lane

Attachment Content-Type Size
separate-shared-state-reset-from-ReScan.patch text/x-diff 24.7 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Amit Kapila 2017-08-30 10:27:55 Re: [HACKERS] [postgresql 10 beta3] unrecognized node type: 90
Previous Message Amit Kapila 2017-08-29 07:37:01 Re: [BUGS] [postgresql 10 beta3] unrecognized node type: 90

Browse pgsql-hackers by date

  From Date Subject
Next Message Bossart, Nathan 2017-08-29 16:47:30 Re: [Proposal] Allow users to specify multiple tables in VACUUM commands
Previous Message David Steele 2017-08-29 16:15:28 OpenFile() Permissions Refactor