Horton

Re: Virtual generated columns

Peter Eisentraut <peter@eisentraut.org>

View thread
From: Peter Eisentraut <peter@eisentraut.org>
To: jian he <jian.universality@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-09-09T06:06:40Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Expand virtual generated columns for ALTER COLUMN TYPE

  2. Eliminate code duplication in replace_rte_variables callbacks

  3. Expand virtual generated columns in the planner

  4. Virtual generated columns

  5. Additional tests for stored generated columns

  6. Improve generated_stored test

  7. Fix handling of CREATE DOMAIN with GENERATED constraint syntax

  8. Add pg_constraint rows for not-null constraints

  9. Put generated_stored test objects in a schema

  10. Rename regress test generated to generated_stored

  11. Small code simplification

  12. Remove useless code

  13. Remove useless initializations

  14. doc: Clarify that pg_attrdef also stores generation expressions

  15. Clean out column-level pg_init_privs entries when dropping tables.

  16. Re-implement the ereport() macro using __VA_ARGS__.

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.