From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Andres Freund <andres(at)2ndquadrant(dot)com> |
Cc: | PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Expanding the use of FLEXIBLE_ARRAY_MEMBER for declarations like foo[1] |
Date: | 2015-02-18 08:15:18 |
Message-ID: | CAB7nPqTi38XMkEy13M2Xw7stSfAqaZmKfb9eKizN2NQ_c+YrHw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Feb 17, 2015 at 9:02 AM, Andres Freund wrote:
> On 2015-02-17 05:51:22 +0900, Michael Paquier wrote:
>> -- inv_api.c uses bytea in an internal structure without putting it at
>> the end of the structure. For clarity I think that we should just use
>> a bytea pointer and do a sufficient allocation for the duration of the
>> lobj manipulation.
>
> Hm. I don't really see the problem tbh.
>
> There's actually is a reason the bytea is at the beginning - the other
> fields are *are* the data part of the bytea (which is just the varlena
> header).
>
>> -- Similarly, tuptoaster.c needed to be patched for toast_save_datum
>
> I'm not a big fan of these two changes. This adds some not that small
> memory allocations to a somewhat hot path. Without a big win in
> clarity. And it doesn't seem to have anything to do with with
> FLEXIBLE_ARRAY_MEMBER.
We could replace those palloc calls with a simple buffer that has a
predefined size, but this somewhat reduces the readability of the
code.
>> There are as well couple of things that are not changed on purpose:
>> - in namespace.h for FuncCandidateList. I tried manipulating it but it
>> resulted in allocation overflow in PortalHeapMemory
>
> Did you change the allocation in FuncnameGetCandidates()? Note the -
> sizeof(Oid) there.
Yeah. Missed it.
>> - I don't think that the t_bits fields in htup_details.h should be
>> updated either.
>
> Why not? Any not broken code should already use MinHeapTupleSize and
> similar macros.
Changing t_bits impacts HeapTupleHeaderData, ReorderBufferTupleBuf and
similarly a couple of redo routines in heapam.c using
HeapTupleHeaderData in a couple of structures not placing it at the
end (compiler complains). We could use for each of them a buffer that
has enough room with sizeof(HeapTupleHeaderData) + MaxHeapTupleSize,
but wouldn't it reduce the readability of the current code? Opinions
welcome.
>> diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
> [...]
> Generally the catalog changes are much less clearly a benefit imo.
OK, let's drop them then.
>> From ad8f54cdb5776146f17d1038bb295b5f13b549f1 Mon Sep 17 00:00:00 2001
>> From: Michael Paquier <michael(at)otacoo(dot)com>
>> Date: Mon, 16 Feb 2015 03:53:38 +0900
>> Subject: [PATCH 3/3] Update varlena in c.h to use FLEXIBLE_ARRAY_MEMBER
>>
>> Places using a variable-length variable not at the end of a structure
>> are changed with workaround, without impacting what those features do.
>
> I vote for rejecting most of this, except a (corrected version) of the
> pg_authid change. Which doesn't really belong to the rest of the patch
> anyway ;)x
Changing bytea to use FLEXIBLE_ARRAY_MEMBER requires those changes, or
at least some changes in this area as something with
FLEXIBLE_ARRAY_MEMBER can only be placed at the end of a structure.
But I guess that we can do fine as well by replacing those structures
with some buffers with a pre-defined size. I'll draft an additional
patch on top of 0001 with all those less-trivial changes implemented.
>> #define VARHDRSZ ((int32) sizeof(int32))
>> diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
>> index e01e6aa..d8789a5 100644
>> --- a/src/include/catalog/pg_authid.h
>> +++ b/src/include/catalog/pg_authid.h
>> @@ -56,8 +56,10 @@ CATALOG(pg_authid,1260) BKI_SHARED_RELATION BKI_ROWTYPE_OID(2842) BKI_SCHEMA_MAC
>> int32 rolconnlimit; /* max connections allowed (-1=no limit) */
>>
>> /* remaining fields may be null; use heap_getattr to read them! */
>> - text rolpassword; /* password, if any */
>> timestamptz rolvaliduntil; /* password expiration time, if any */
>> +#ifdef CATALOG_VARLEN
>> + text rolpassword; /* password, if any */
>> +#endif
>> } FormData_pg_authid;
>
> That change IIRC is wrong, because it'll make rolvaliduntil until NOT
> NULL (any column that's fixed width and has only fixed with columns
> before it is marked as such).
This sounds better as a separate patch...
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Kyotaro HORIGUCHI | 2015-02-18 08:34:41 | Re: pg_basebackup may fail to send feedbacks. |
Previous Message | Kouhei Kaigai | 2015-02-18 08:13:10 | Re: Combining Aggregates |