Feedback-ID: ie0a040ee:Fastmail
Message-ID: <1841bdbe-85a5-4f9d-b8ee-7f8790aa33ff@eisentraut.org>
Date: Mon, 9 Sep 2024 08:06:40 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Virtual generated columns
To: jian he <jian.universality@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <a368248e-69e4-40be-9c07-6c3b5880b0a6@eisentraut.org>
 <ffc0cb54-0666-4820-9baa-49d839007eef@eisentraut.org>
 <CACJufxE+E-iYmBnZVZHiYA+WpyZZVv7BfiBLpo=T70EZHDU9rw@mail.gmail.com>
 <fd6d260b-a401-4355-8688-e3bcb7e93527@eisentraut.org>
 <CACJufxGh=XX7q5xTci---HikSJsOwP7_qCNyyDHdp2YL4NLZCg@mail.gmail.com>
 <CACJufxHxoz_KVQxcQ=cSvJc+PNbh=FaHbcMZCOisoOaGAgjy4w@mail.gmail.com>
 <732a9543-0312-4f0c-a471-857b507a3546@eisentraut.org>
 <ebd1e0e9-115f-4dfa-98b8-e9b8e2f9eb2d@eisentraut.org>
 <CAEZATCV=RX0M58-uqSR6tFjwQAFnEcx3hvW-9ZZfRQJLg5LzRw@mail.gmail.com>
 <0f811f71-8c86-4e1b-b3bb-6a535674a440@eisentraut.org>
 <CAEZATCU3q=znAL7X1aSxuMpjfSyB1cT5j398HPb9ix9ci_HUqA@mail.gmail.com>
 <cd7c0fea-6c4b-4bdd-af3b-4d4dabb32ca1@eisentraut.org>
 <CACJufxHf5cfSQAsd9VgjFnk=RUFpOLiFE3C+C1s14qHxyKNhdQ@mail.gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <CACJufxHf5cfSQAsd9VgjFnk=RUFpOLiFE3C+C1s14qHxyKNhdQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/1841bdbe-85a5-4f9d-b8ee-7f8790aa33ff%40eisentraut.org>
Precedence: bulk

On 05.09.24 10:27, jian he wrote:
> On Wed, Sep 4, 2024 at 4:40 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>>
>> Here is an implementation of this.  It's much nicer!  It also appears to
>> fix all the additional test cases that have been presented.  (I haven't
>> integrated them into the patch set yet.)
>>
>> I left the 0001 patch alone for now and put the new rewriting
>> implementation into 0002.  (Unfortunately, the diff is kind of useless
>> for visual inspection.)  Let me know if this matches what you had in
>> mind, please.  Also, is this the right place in fireRIRrules()?
> 
> hi. some minor issues.
> 
> in get_dependent_generated_columns we can
> 
>              /* skip if not generated column */
>              if (!TupleDescAttr(tupdesc, defval->adnum - 1)->attgenerated)
>                  continue;
> change to
>              /* skip if not generated stored column */
>              if (!(TupleDescAttr(tupdesc, defval->adnum -
> 1)->attgenerated == ATTRIBUTE_GENERATED_STORED))
>                  continue;

I need to study more what to do with this function.  I'm not completely 
sure whether this should apply only to stored generated columns.

> in ExecInitStoredGenerated
> "if ((tupdesc->constr && tupdesc->constr->has_generated_stored)))"
> is true.
> then later we finish the loop
> (for (int i = 0; i < natts; i++) loop)
> 
> we can "Assert(ri_NumGeneratedNeeded > 0)"
> so we can ensure once has_generated_stored flag is true,
> then we should have at least one stored generated attribute.

This is technically correct, but this code isn't touched by this patch, 
so I don't think it belongs here.

> similarly, in expand_generated_columns_internal
> we can aslo add "Assert(list_length(tlist) > 0);"
> above
> node = ReplaceVarsFromTargetList(node, rt_index, 0, rte, tlist,
> REPLACEVARS_CHANGE_VARNO, rt_index, NULL);

Ok, I'll add that.

> @@ -2290,7 +2291,9 @@ ExecBuildSlotValueDescription(Oid reloid,
> if (table_perm || column_perm)
> {
> - if (slot->tts_isnull[i])
> + if (att->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
> + val = "virtual";
> + else if (slot->tts_isnull[i])
>      val = "null";
> else
> {
> Oid  foutoid;
> bool typisvarlena;
> getTypeOutputInfo(att->atttypid, &foutoid, &typisvarlena);
> val = OidOutputFunctionCall(foutoid, slot->tts_values[i]);
> }
> 
> we can add Assert here, if i understand it correctly, like
>   if (att->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
> {
> Assert(slot->tts_isnull[i]);
>   val = "virtual";
> }

Also technically correct, but I don't see what benefit this would bring. 
  The code guarded by that assert would not make use of the thing being 
asserted.