MIME-Version: 1.0
References: 
 <CAFj8pRC+hPCc2X88xC=pTJoqmVPApDsageZOMyqaxi5788WxHA@mail.gmail.com>
 <CAFj8pRDJ9cq00VYSHxs6LsoHNWjhYXyWWBtV6UgeWwhs0AHa9A@mail.gmail.com>
 <CAFj8pRBPXTcw_3fpKtgVthV2+9rZGhxitZ40DnAwCrK601TZZg@mail.gmail.com>
 <ndtfl4tsnpkb7m7hwvnmlpsascpgd3a7xvjmjhtxffsbrgygtm@4du6zsmnnwq5>
 <CAFj8pRAu4XvNCGu1751t=2YEqLqTjDA3FavMExm2S0KYQq=DdQ@mail.gmail.com>
 <CAFj8pRAsEoeZv0HEnA8CKgFKDSQ-wYw18Os1vdksWCV7ez2bVw@mail.gmail.com>
 <3chredgnjcmccym2kczawfih226b4ac6co7p6z4jeofevrcosi@mrsxkx2x2c65>
 <CAFj8pRBoWPDTOwn5FmMzc+1qiopw+N04U26nviOdF61fs8A2wQ@mail.gmail.com>
 <stckyvkl4yyzvgjsaawojs3xikke7mmds5bhv7l7qerclywywk@h4v4n43xm6u2>
 <CAFj8pRB_E1GM_YGT-ti4bXka6mhLdAAFeTe+BHgHFYC+qb-76g@mail.gmail.com>
 <20241120201313.t4wbhld4ktgielaf@erthalion.local>
 <CAFj8pRBWqEb8i6WmrF_Xh64=48GtisKijgczMv7HTTpe4GswuA@mail.gmail.com>
 <CAFj8pRAry0esQiHcK=6BwwFKDY0zanug6k07CEQzRPBqZ6iW0Q@mail.gmail.com>
 <CACJufxFNjKrmyEi9SLfPCq4c9GUN+5eoOtbZwBPq9eKoO8REUw@mail.gmail.com>
 <CAFj8pRALQ-j-Dz3R1ivCoXut8LEhN+kSa7U8Gshucdv5zU3AfQ@mail.gmail.com>
 <CACJufxG7LvaNbF8ZSCcxOVUbm9W=KGjD=h_Wk+5imMw4s_2QxA@mail.gmail.com>
 <CAFj8pRBY_2awVdER5piyy_JPqsU1Sgr4HLO-v6C1nUS3dJnang@mail.gmail.com>
 <CACJufxEM=BLEn6YfgGonM7yuXMn7iqQJcH5PnDVbajWKanynfg@mail.gmail.com>
 <CAFj8pRBxA868TpLDe9ofXdpVUNmHY8pzkxrjbZ0obCe0g+YZ-Q@mail.gmail.com>
 <CACJufxEb1prZo4G4BhYym1VqKHaPVUv3SoggTR4uCqgCRY7Q-Q@mail.gmail.com>
 <CAFj8pRDK2heEhQNcwD=tcKpG5YyPu7zOz2jdRW5A5QfoSUEi_g@mail.gmail.com>
 <CACJufxEk16ArmFuTV9VYi6DJaZ+9Jrbt0-u0Zuov7sk-vpg_Dg@mail.gmail.com>
 <CAFj8pRAaoPCc=n_TPpWdOvzEdc96xfJZR8W7PphMLxLbGxAq-Q@mail.gmail.com>
 <CAFj8pRAzFCbTwqjaEvkvB0VBa9ofSEYxnv8wM3pHZTGtrRK0yQ@mail.gmail.com>
In-Reply-To: 
 <CAFj8pRAzFCbTwqjaEvkvB0VBa9ofSEYxnv8wM3pHZTGtrRK0yQ@mail.gmail.com>
From: jian he <jian.universality@gmail.com>
Date: Fri, 3 Jan 2025 15:18:22 +0800
Message-ID: 
 <CACJufxFwxAvJL944UQKwxV1YuM3GQNTbPRQ6LwtioSKBfpMN2A@mail.gmail.com>
Subject: Re: Re: proposal: schema variables
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>,
 Laurenz Albe <laurenz.albe@cybertec.at>,
	Erik Rijkers <er@xs4all.nl>, Michael Paquier <michael@paquier.xyz>,
	Amit Kapila <amit.kapila16@gmail.com>, DUVAL REMI <REMI.DUVAL@cheops.fr>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="000000000000a1aade062ac817c8"
Archived-At: 
 <https://www.postgresql.org/message-id/CACJufxFwxAvJL944UQKwxV1YuM3GQNTbPRQ6LwtioSKBfpMN2A%40mail.gmail.com>
Precedence: bulk

--000000000000a1aade062ac817c8
Content-Type: text/plain; charset="UTF-8"

hi.

in the function svariableStartupReceiver all these "ereport(ERROR"
cannot happen,
since transformLetStmt already did all the heavy work.
base on https://www.postgresql.org/docs/current/error-message-reporting.html
all these "ereport(ERROR," in the svariableStartupReceiver can be
simplified as "elog(ERROR,"
or Assert.


After standard_ExecutorStart->InitPlan, queryDesc.tupDesc will not
include attr->attisdropped is true scarenio.
In standard_ExecutorStart, I added the following code then ran the
regress test again to prove my point.

standard_ExecutorStart
    /*
     * Initialize the plan state tree
     */
    InitPlan(queryDesc, eflags);
    for (int i = 0; i < queryDesc->tupDesc->natts; i++)
    {
        Form_pg_attribute attr = TupleDescAttr(queryDesc->tupDesc, i);
        if (attr->attisdropped)
        {
            elog(INFO, "some attribute is dropped queryDesc->operation
is %d", queryDesc->operation);
        }
    }
    MemoryContextSwitchTo(oldcontext);
-------------------------
svariableStartupReceiver parameter typeinfo is from queryDesc->tupDesc
So I think svariableStartupReceiver, typeinfo->natts will always equal one.
therefore SVariableState.slot_offset is not necessary.

overall, i think svariableStartupReceiver can be simplified as the following:

static void
svariableStartupReceiver(DestReceiver *self, int operation, TupleDesc typeinfo)
{
    SVariableState *myState = (SVariableState *) self;
    int            natts = typeinfo->natts;
    Form_pg_attribute attr;
    LOCKTAG        locktag PG_USED_FOR_ASSERTS_ONLY;
    Assert(myState->pub.mydest == DestVariable);
    Assert(OidIsValid(myState->varid));
    Assert(SearchSysCacheExists1(VARIABLEOID, myState->varid));
#ifdef USE_ASSERT_CHECKING
    SET_LOCKTAG_OBJECT(locktag,
                       MyDatabaseId,
                       VariableRelationId,
                       myState->varid,
                       0);
    Assert(LockHeldByMe(&locktag, AccessShareLock, false));
#endif
    Assert(natts == 1);
    attr = TupleDescAttr(typeinfo, 0);
    myState->need_detoast = attr->attlen == -1;
    myState->rows = 0;
}

I've attached the file containing the changes I mentioned earlier.

-------------------------<<>>>-------------------------------
Overall, 0001 and 0002 the doc looks good to me now.
The following are only some minor issues I came up with.

In Table 5.1. ACL Privilege Abbreviations
<table id="privilege-abbrevs-table">
<title><acronym>ACL</acronym> Privilege Abbreviations</title>

<literal>VARIABLE</literal> (occurred 3 times)
should be
<literal>SESSION VARIABLE</literal>
?

doc/src/sgml/glossary.sgml
I want to do minor tweak. from
    <para>
     A persistent database object that holds a value in session memory.  This
     value is private to each session and is released when the session ends.
     Read or write access to session variables is controlled by privileges,
     similar to other database objects.
    </para>
to
    <para>
     A persistent database object that holds a value in session memory.  This
     value is private to each session and is reset to default value
(null) when the session ends.
     Read or write access to session variables is controlled by access
privileges,
     similar to other database objects.
    </para>

in let.sgml.
  <para>
   Example:
<programlisting>
CREATE VARIABLE myvar AS integer;
LET myvar = 10;
LET myvar = (SELECT sum(val) FROM tab);
</programlisting>
  </para>

it should be
 <refsect1>
  <title>Examples</title>
...your example code
 </refsect1>

--000000000000a1aade062ac817c8
Content-Type: application/octet-stream;
	name="v1-0001-refactoring-svariableStartupReceiver.no-cfbot"
Content-Disposition: attachment;
	filename="v1-0001-refactoring-svariableStartupReceiver.no-cfbot"
Content-Transfer-Encoding: base64
Content-ID: <f_m5gf7x440>
X-Attachment-Id: f_m5gf7x440

RnJvbSA0MmU4MzQ0OTQ3NmI1YWRiZGQyNGNhOWEwOTNlMTA4NDBjYzQ3Zjc5IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBqaWFuIGhlIDxqaWFuLnVuaXZlcnNhbGl0eUBnbWFpbC5jb20+
CkRhdGU6IEZyaSwgMyBKYW4gMjAyNSAxNTowOTowNiArMDgwMApTdWJqZWN0OiBbUEFUQ0ggdjEg
MS8xXSByZWZhY3RvcmluZyBzdmFyaWFibGVTdGFydHVwUmVjZWl2ZXIKCi0tLQogc3JjL2JhY2tl
bmQvZXhlY3V0b3Ivc3ZhcmlhYmxlUmVjZWl2ZXIuYyB8IDY2ICsrKystLS0tLS0tLS0tLS0tLS0t
LS0tLQogMSBmaWxlIGNoYW5nZWQsIDkgaW5zZXJ0aW9ucygrKSwgNTcgZGVsZXRpb25zKC0pCgpk
aWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvZXhlY3V0b3Ivc3ZhcmlhYmxlUmVjZWl2ZXIuYyBiL3Ny
Yy9iYWNrZW5kL2V4ZWN1dG9yL3N2YXJpYWJsZVJlY2VpdmVyLmMKaW5kZXggYzRjZmY0NGVjZi4u
YTM1NzRiNDY4NSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvZXhlY3V0b3Ivc3ZhcmlhYmxlUmVj
ZWl2ZXIuYworKysgYi9zcmMvYmFja2VuZC9leGVjdXRvci9zdmFyaWFibGVSZWNlaXZlci5jCkBA
IC0yOCwxOSArMjgsMTYgQEAKIC8qCiAgKiBUaGlzIERlc3RSZWNlaXZlciBpcyB1c2VkIGJ5IHRo
ZSBMRVQgY29tbWFuZCBmb3Igc3RvcmluZyB0aGUgcmVzdWx0IHRvIGEKICAqIHNlc3Npb24gdmFy
aWFibGUuICBUaGUgcmVzdWx0IGhhcyB0byBoYXZlIG9ubHkgb25lIHR1cGxlIHdpdGggb25seSBv
bmUKLSAqIG5vbi1kZWxldGVkIGF0dHJpYnV0ZS4gIFRoZSByb3cgY291bnRlciAoZmllbGQgInJv
d3MiKSBpcyBpbmNyZW1lbnRlZAorICogYXR0cmlidXRlLiAgVGhlIHJvdyBjb3VudGVyIChmaWVs
ZCAicm93cyIpIGlzIGluY3JlbWVudGVkCiAgKiBhZnRlciByZWNlaXZpbmcgYSByb3csIGFuZCBh
biBlcnJvciBpcyByYWlzZWQgd2hlbiB0aGVyZSBhcmUgbm8gcm93cyBvcgotICogdGhlcmUgYXJl
IG1vcmUgdGhhbiBvbmUgcmVjZWl2ZWQgcm93cy4gIEJlY2F1c2UgYSByZWNlaXZlZCB0dXBsZSBj
YW4gaGF2ZQotICogZGVsZXRlZCBhdHRyaWJ1dGVzLCB3ZSBuZWVkIHRvIGZpbmQgdGhlIGZpcnN0
IG5vbi1kZWxldGVkIGF0dHJpYnV0ZQotICogKGZpZWxkICJzbG90X29mZnNldCIpLiAgVGhlIHZh
bHVlIGlzIGRldG9hc3RlZCBiZWZvcmUgc3RvcmluZyBpdCBpbiB0aGUKLSAqIHNlc3Npb24gdmFy
aWFibGUuCisgKiB0aGVyZSBhcmUgbW9yZSB0aGFuIG9uZSByZWNlaXZlZCByb3dzLgorICogVGhl
IHZhbHVlIGlzIGRldG9hc3RlZCBiZWZvcmUgc3RvcmluZyBpdCBpbiB0aGUgc2Vzc2lvbiB2YXJp
YWJsZS4KICAqLwogdHlwZWRlZiBzdHJ1Y3QKIHsKIAlEZXN0UmVjZWl2ZXIgcHViOwogCU9pZAkJ
CXZhcmlkOwogCWJvb2wJCW5lZWRfZGV0b2FzdDsJCS8qIGRvIHdlIG5lZWQgdG8gZGV0b2FzdCB0
aGUgYXR0cmlidXRlPyAqLwotCWludAkJCXNsb3Rfb2Zmc2V0OwkJLyogcG9zaXRpb24gb2Ygbm9u
LWRlbGV0ZWQgYXR0cmlidXRlICovCiAJaW50CQkJcm93czsJCQkJLyogcm93IGNvdW50ZXIgKi8K
IH0gU1ZhcmlhYmxlU3RhdGU7CiAKQEAgLTUyLDggKzQ5LDcgQEAgc3ZhcmlhYmxlU3RhcnR1cFJl
Y2VpdmVyKERlc3RSZWNlaXZlciAqc2VsZiwgaW50IG9wZXJhdGlvbiwgVHVwbGVEZXNjIHR5cGVp
bmZvKQogewogCVNWYXJpYWJsZVN0YXRlICpteVN0YXRlID0gKFNWYXJpYWJsZVN0YXRlICopIHNl
bGY7CiAJaW50CQkJbmF0dHMgPSB0eXBlaW5mby0+bmF0dHM7Ci0JaW50CQkJb3V0Y29scyA9IDA7
Ci0JaW50CQkJaTsKKwlGb3JtX3BnX2F0dHJpYnV0ZSBhdHRyOwogCUxPQ0tUQUcJCWxvY2t0YWcg
UEdfVVNFRF9GT1JfQVNTRVJUU19PTkxZOwogCiAJQXNzZXJ0KG15U3RhdGUtPnB1Yi5teWRlc3Qg
PT0gRGVzdFZhcmlhYmxlKTsKQEAgLTcxLDUzICs2Nyw5IEBAIHN2YXJpYWJsZVN0YXJ0dXBSZWNl
aXZlcihEZXN0UmVjZWl2ZXIgKnNlbGYsIGludCBvcGVyYXRpb24sIFR1cGxlRGVzYyB0eXBlaW5m
bykKIAlBc3NlcnQoTG9ja0hlbGRCeU1lKCZsb2NrdGFnLCBBY2Nlc3NTaGFyZUxvY2ssIGZhbHNl
KSk7CiAKICNlbmRpZgotCi0JZm9yIChpID0gMDsgaSA8IG5hdHRzOyBpKyspCi0JewotCQlGb3Jt
X3BnX2F0dHJpYnV0ZSBhdHRyID0gVHVwbGVEZXNjQXR0cih0eXBlaW5mbywgaSk7Ci0JCU9pZAkJ
CXR5cGlkOwotCQlPaWQJCQljb2xsaWQ7Ci0JCWludDMyCQl0eXBtb2Q7Ci0KLQkJaWYgKGF0dHIt
PmF0dGlzZHJvcHBlZCkKLQkJCWNvbnRpbnVlOwotCi0JCWlmICgrK291dGNvbHMgPiAxKQotCQkJ
Y29udGludWU7Ci0KLQkJZ2V0X3Nlc3Npb25fdmFyaWFibGVfdHlwZV90eXBtb2RfY29sbGlkKG15
U3RhdGUtPnZhcmlkLAotCQkJCQkJCQkJCQkJJnR5cGlkLAotCQkJCQkJCQkJCQkJJnR5cG1vZCwK
LQkJCQkJCQkJCQkJCSZjb2xsaWQpOwotCi0JCS8qCi0JCSAqIERvdWJsZSBjaGVjayAtIHRoZSB0
eXBlIGFuZCB0eXBtb2Qgb2YgdGFyZ2V0IHZhcmlhYmxlIHNob3VsZCBiZSB0aGUKLQkJICogc2Ft
ZSBhcyB0aGUgdHlwZSBhbmQgdHlwbW9kIG9mIGFzc2lnbm1lbnQgZXhwcmVzc2lvbi4gIFRoZQot
CQkgKiBleHByZXNzaW9uIHNob3VsZCBiZSB3cmFwcGVkIGJ5IGEgY2FzdCB0byB0aGUgdGFyZ2V0
IHR5cGUvdHlwbW9kLgotCQkgKi8KLQkJaWYgKGF0dHItPmF0dHR5cGlkICE9IHR5cGlkIHx8Ci0J
CQkoYXR0ci0+YXR0dHlwbW9kID49IDAgJiYKLQkJCSBhdHRyLT5hdHR0eXBtb2QgIT0gdHlwbW9k
KSkKLQkJCWVyZXBvcnQoRVJST1IsCi0JCQkJCShlcnJjb2RlKEVSUkNPREVfREFUQVRZUEVfTUlT
TUFUQ0gpLAotCQkJCQkgZXJybXNnKCJ0YXJnZXQgc2Vzc2lvbiB2YXJpYWJsZSBpcyBvZiB0eXBl
ICVzIgotCQkJCQkJCSIgYnV0IGV4cHJlc3Npb24gaXMgb2YgdHlwZSAlcyIsCi0JCQkJCQkJZm9y
bWF0X3R5cGVfd2l0aF90eXBlbW9kKHR5cGlkLCB0eXBtb2QpLAotCQkJCQkJCWZvcm1hdF90eXBl
X3dpdGhfdHlwZW1vZChhdHRyLT5hdHR0eXBpZCwKLQkJCQkJCQkJCQkJCQkgYXR0ci0+YXR0dHlw
bW9kKSkpKTsKLQotCQlteVN0YXRlLT5uZWVkX2RldG9hc3QgPSBhdHRyLT5hdHRsZW4gPT0gLTE7
Ci0JCW15U3RhdGUtPnNsb3Rfb2Zmc2V0ID0gaTsKLQl9Ci0KLQlpZiAob3V0Y29scyAhPSAxKQot
CQllcmVwb3J0KEVSUk9SLAotCQkJCShlcnJjb2RlKEVSUkNPREVfU1lOVEFYX0VSUk9SKSwKLQkJ
CQkgZXJybXNnX3BsdXJhbCgiYXNzaWdubWVudCBleHByZXNzaW9uIHJldHVybmVkICVkIGNvbHVt
biIsCi0JCQkJCQkJICAgImFzc2lnbm1lbnQgZXhwcmVzc2lvbiByZXR1cm5lZCAlZCBjb2x1bW5z
IiwKLQkJCQkJCQkgICBvdXRjb2xzLAotCQkJCQkJCSAgIG91dGNvbHMpKSk7Ci0KKwlBc3NlcnQo
bmF0dHMgPT0gMSk7CisJYXR0ciA9IFR1cGxlRGVzY0F0dHIodHlwZWluZm8sIDApOworCW15U3Rh
dGUtPm5lZWRfZGV0b2FzdCA9IGF0dHItPmF0dGxlbiA9PSAtMTsKIAlteVN0YXRlLT5yb3dzID0g
MDsKIH0KIApAQCAtMTM1LDggKzg3LDggQEAgc3ZhcmlhYmxlUmVjZWl2ZVNsb3QoVHVwbGVUYWJs
ZVNsb3QgKnNsb3QsIERlc3RSZWNlaXZlciAqc2VsZikKIAkvKiBtYWtlIHN1cmUgdGhlIHR1cGxl
IGlzIGZ1bGx5IGRlY29uc3RydWN0ZWQgKi8KIAlzbG90X2dldGFsbGF0dHJzKHNsb3QpOwogCi0J
dmFsdWUgPSBzbG90LT50dHNfdmFsdWVzW215U3RhdGUtPnNsb3Rfb2Zmc2V0XTsKLQlpc251bGwg
PSBzbG90LT50dHNfaXNudWxsW215U3RhdGUtPnNsb3Rfb2Zmc2V0XTsKKwl2YWx1ZSA9IHNsb3Qt
PnR0c192YWx1ZXNbMF07CisJaXNudWxsID0gc2xvdC0+dHRzX2lzbnVsbFswXTsKIAogCWlmICht
eVN0YXRlLT5uZWVkX2RldG9hc3QgJiYgIWlzbnVsbCAmJiBWQVJBVFRfSVNfRVhURVJOQUwoRGF0
dW1HZXRQb2ludGVyKHZhbHVlKSkpCiAJewotLSAKMi4zNC4xCgo=
--000000000000a1aade062ac817c8--