Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qdtOQ-0055p6-NO for pgsql-hackers@arkaria.postgresql.org; Wed, 06 Sep 2023 14:20:30 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1qdtOP-00Ao6u-70 for pgsql-hackers@arkaria.postgresql.org; Wed, 06 Sep 2023 14:20:28 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qdtOO-00Ao6k-S3 for pgsql-hackers@lists.postgresql.org; Wed, 06 Sep 2023 14:20:28 +0000 Received: from mail-lf1-x133.google.com ([2a00:1450:4864:20::133]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1qdtOF-003Qio-Rk for pgsql-hackers@postgresql.org; Wed, 06 Sep 2023 14:20:27 +0000 Received: by mail-lf1-x133.google.com with SMTP id 2adb3069b0e04-50078e52537so6266016e87.1 for ; Wed, 06 Sep 2023 07:20:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694010018; x=1694614818; darn=postgresql.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=1DpdS1UGpCmLXugioTqRrVDo1RYv7vRL9kqUT32Morg=; b=DMZPE4lHv2GooFCfXk22v8/s6wegCNVJ4YIa0J3zkWUvRG8VHY3o4czP39ffjteBQC Bh4gcsZ9wnre9Kldh+twREaYCrgE2UrRfQ/fmzzQy3YJCKtoxk/lUlAMjEoMKTu54eQV N6nS+uwpeFZzXFW61p+go22GvcQnJ/yqo2uYVeiOH7ZZUJVk51Wki5x196IPZpYIQ8NC rR6yYszpyYOTNntGW71wzD95Wg/5x4E7iNAP/I1fvrTuw7jzYn9QvU1vc8tsINime5zp WFGyHVDBqucgZQ3TJakeKISvoZ16perrfqiOf4AtpkafhhknWW+KWnGRoaDGmcdbWoaQ 67Yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1694010018; x=1694614818; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1DpdS1UGpCmLXugioTqRrVDo1RYv7vRL9kqUT32Morg=; b=I5F8O9fBLSd3XKK01ZwFCMPZML65U5hkRhCAjUUuXRHmH2YNXlcMMw0R+cIPSc6qi1 Hj7DkTcspJ00DdFdv6R0Jx/i6wJHpAROGoatulDMyczyKJfGytk/YgF2yNdAZK1Xtg5O ZhZGeCsOp3lcW1zvjQL0wgzUc5pYKzsCl04g3mNmIe8J9f8GeZBQLGSHo02ry+5gNW9W ugP5wZ9pb7NhwOjEwwVSPPglVmy1Oj5N1lV8cNCULD2Io4zZ/+mJbN0QSnVl0W66SDm8 P+vpeXhG1ZPfIZzMV8vYDGbjIUMZb3nnS9mC2WbcdxVBCWSLQzU3hI4W5MCnopc0vyQC e4Xw== X-Gm-Message-State: AOJu0Yx2axI3YlWseVFEOwvFiVhP0tIMNXAmBoypSGt+kICzpc0H3blI I4xkcSxPy+cJObs4r3DkrqUBJyT9K1RgzLTj210= X-Google-Smtp-Source: AGHT+IERU5Eio1e5hvSDMSJNDrN0fiRzjO8pdqi4fLAJjyTDwTADcugKe+VLF6gYzdeMbJne75OhlSpQf2f3HN08r6A= X-Received: by 2002:ac2:5f8b:0:b0:500:ac10:1641 with SMTP id r11-20020ac25f8b000000b00500ac101641mr2133619lfe.46.1694010018233; Wed, 06 Sep 2023 07:20:18 -0700 (PDT) MIME-Version: 1.0 References: <20221221101846.7zsi7lscnm5ediik@alvherre.pgsql> <1350682.1671635927@sss.pgh.pa.us> <4191508.1674157166@sss.pgh.pa.us> <349124.1674185509@sss.pgh.pa.us> <20230207180855.xy5m4puwh5gzd7xy@awork3.anarazel.de> <1274885.1680558101@sss.pgh.pa.us> <9D23D8BB-83EE-451E-95C1-CF42ADB76869@yesql.se> In-Reply-To: From: Robert Haas Date: Wed, 6 Sep 2023 10:20:06 -0400 Message-ID: Subject: Re: generic plans and "initial" pruning To: Amit Langote Cc: Alvaro Herrera , Andres Freund , Daniel Gustafsson , David Rowley , Jacob Champion , PostgreSQL Hackers , Thom Brown , Tom Lane Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk On Wed, Sep 6, 2023 at 5:12=E2=80=AFAM Amit Langote wrote: > Attached updated patches. Thanks for the review. I think 0001 looks ready to commit. I'm not sure that the commit message needs to mention future patches here, since this code cleanup seems like a good idea regardless, but if you feel otherwise, fair enough. On 0002, some questions: - In ExecEndLockRows, is the call to EvalPlanQualEnd a concern? i.e. Does that function need any adjustment? - In ExecEndMemoize, should there be a null-test around MemoryContextDelete(node->tableContext) as we have in ExecEndRecursiveUnion, ExecEndSetOp, etc.? I wonder how we feel about setting pointers to NULL after freeing the associated data structures. The existing code isn't consistent about doing that, and making it do so would be a fairly large change that would bloat this patch quite a bit. On the other hand, I think it's a good practice as a general matter, and we do do it in some ExecEnd functions. On 0003, I have some doubt about whether we really have all the right design decisions in detail here: - Why have this weird rule where sometimes we return NULL and other times the planstate? Is there any point to such a coding rule? Why not just always return the planstate? - Is there any point to all of these early exit cases? For example, in ExecInitBitmapAnd, why exit early if initialization fails? Why not just plunge ahead and if initialization failed the caller will notice that and when we ExecEndNode some of the child node pointers will be NULL but who cares? The obvious disadvantage of this approach is that we're doing a bunch of unnecessary initialization, but we're also speeding up the common case where we don't need to abort by avoiding a branch that will rarely be taken. I'm not quite sure what the right thing to do is here. - The cases where we call ExecGetRangeTableRelation or ExecOpenScanRelation are a bit subtler ... maybe initialization that we're going to do later is going to barf if the tuple descriptor of the relation isn't what we thought it was going to be. In that case it becomes important to exit early. But if that's not actually a problem, then we could apply the same principle here also -- don't pollute the code with early-exit cases, just let it do its thing and sort it out later. Do you know what the actual problems would be here if we didn't exit early in these cases? - Depending on the answers to the above points, one thing we could think of doing is put an early exit case into ExecInitNode itself: if (unlikely(!ExecPlanStillValid(whatever)) return NULL. Maybe Andres or someone is going to argue that that checks too often and is thus too expensive, but it would be a lot more maintainable than having similar checks strewn throughout the ExecInit* functions. Perhaps it deserves some thought/benchmarking. More generally, if there's anything we can do to centralize these checks in fewer places, I think that would be worth considering. The patch isn't terribly large as it stands, so I don't necessarily think that this is a critical issue, but I'm just wondering if we can do better. I'm not even sure that it would be too expensive to just initialize the whole plan always, and then just do one test at the end. That's not OK if the changed tuple descriptor (or something else) is going to crash or error out in a funny way or something before initialization is completed, but if it's just going to result in burning a few CPU cycles in a corner case, I don't know if we should really care. - The "At this point" comments don't give any rationale for why we shouldn't have received any such invalidation messages. That makes them fairly useless; the Assert by itself clarifies that you think that case shouldn't happen. The comment's job is to justify that claim. --=20 Robert Haas EDB: http://www.enterprisedb.com