public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tomas Vondra <[email protected]>
To: Amit Langote <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: David Rowley <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Thom Brown <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: generic plans and "initial" pruning
Date: Thu, 5 Dec 2024 15:07:12 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+HiwqE2FfJfH=siLiR3kJ13tmXZORAGTWsZc2r52o1_5BDv+g@mail.gmail.com>
References: <CA+HiwqFpZ80UJKr4tZus4Omgg7YESzFXKSwSHRW2Ap2=XSVyUA@mail.gmail.com>
<CA+HiwqH9u1RWn9OEa=VQQpJagB0hDLCY+=fSyBC4ZkeU6Gg2HA@mail.gmail.com>
<CA+HiwqFMWt2MQVqhp2rZA8=ugPVD=5uW10QCdK_NpoyWyFLe-g@mail.gmail.com>
<CA+HiwqGBpw_JNwkwZjQ2YaqTWrDjn9L5jpuc+nS8=a55SPD+UA@mail.gmail.com>
<CA+HiwqFGz2uShfU=qtack9gii6Kzyjv1V66tJJBYBN8Acb4uTA@mail.gmail.com>
<CA+HiwqE7+iwMH4NYtFi28Pt9fT_gRW+Gt-=CvOX=Pkquo=AN8w@mail.gmail.com>
<CA+TgmobO_6irkJGkzkxHTR=kza_CG+0idAhFUWqGfXCVQQmuPg@mail.gmail.com>
<CA+HiwqH45ZCQkWoLzjOyS6bQ9QsF7yDCKVwiEUtB_RwPFwLmQg@mail.gmail.com>
<CA+HiwqHRRFQN6yZ54fBydOTM6ncqZBCmewZ6n519RjRdDsO44g@mail.gmail.com>
<[email protected]>
<CA+HiwqH8N-SxEB6SysEBsYNgV_KJs66k9Z2SNmqVzbBP-60yWg@mail.gmail.com>
<[email protected]>
<CA+HiwqEmG9YCQvG6uux7sO=jKFSAW6hA4Ea-ymfD+JhJAe4PWQ@mail.gmail.com>
<CA+HiwqE2FfJfH=siLiR3kJ13tmXZORAGTWsZc2r52o1_5BDv+g@mail.gmail.com>
On 12/5/24 12:28, Amit Langote wrote:
> On Thu, Dec 5, 2024 at 3:53 PM Amit Langote <[email protected]> wrote:
>> On Thu, Dec 5, 2024 at 2:20 AM Tomas Vondra <[email protected]> wrote:
>>> Sure, changing the APIs is allowed, I'm just wondering if maybe there
>>> might be a way to not have this issue, or at least notice the missing
>>> call early.
>>>
>>> I haven't tried, wouldn't it be better to modify ExecutorStart() to do
>>> the retries internally? I mean, the extensions wouldn't need to check if
>>> the plan is still valid, ExecutorStart() would take care of that. Yeah,
>>> it might need some new arguments, but that's more obvious.
>>
>> One approach could be to move some code from standard_ExecutorStart()
>> into ExecutorStart(). Specifically, the code responsible for setting
>> up enough state in the EState to perform ExecDoInitialPruning(), which
>> takes locks that might invalidate the plan. If the plan does become
>> invalid, the hook and standard_ExecutorStart() are not called.
>> Instead, the caller, ExecutorStartExt() in this case, creates a new
>> plan.
>>
>> This avoids the need to add ExecPlanStillValid() checks anywhere,
>> whether in core or extension code. However, it does mean accessing the
>> PlannedStmt earlier than InitPlan(), but the current placement of the
>> code is not exactly set in stone.
>
> I tried this approach and found that it essentially disables testing
> of this patch using the delay_execution module, which relies on the
> ExecutorStart_hook(). The way the testing works is that the hook in
> delay_execution.c pauses the execution of a cached plan to allow a
> concurrent session to drop an index referenced in the plan. When
> unpaused, execution initialization resumes by calling
> standard_ExecutorStart(). At this point, obtaining the lock on the
> partition whose index has been dropped invalidates the plan, which the
> hook detects and reports. It then also reports the successful
> re-execution of an updated plan that no longer references the dropped
> index. Hmm.
>
It's not clear to me why the change disables this testing, and I can't
try without a patch. Could you explain?
thanks
--
Tomas Vondra
view thread (66+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: generic plans and "initial" pruning
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox