Prev: PG_MODULE_MAGIC checks and pg_migrator
Next: Streaming replication, loose ends
From: Robert Haas on 19 Jan 2010 15:06
On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(a)gmail.com> wrote:
> ***************
> *** 152,158 **** CATALOG(pg_attribute,1249) BKI_BOOTSTRAP
> BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
>        aclitem         attacl[1];
>
>        /* Column-level options */
> !       aclitem         attoptions[1];
>  } FormData_pg_attribute;
>
>  /*
> --- 152,158 ----
>        aclitem         attacl[1];
>
>        /* Column-level options */
> !       text            attoptions[1];
>  } FormData_pg_attribute;
>
>  /*

Unfortunately this change (which is obviously correct and necessary)
breaks the build on src/backend/catalog/heap.c with:

heap.c:122: error: missing braces around initializer
heap.c:122: error: (near initialization for ‘a1.attoptions[0]’)

....repeated of the 7 hard-coded descriptors. Sadly I'm not quite sure
what to use instead. I can't find any examples of static initializers
for a varlena (which text is). However, I think that it doesn't
actually matter how that gets initialized, because I think only the
fixed-size portion is ever examined, so perhaps I can just leave off
the attoptions and attacl initializers altogether.

Whatever we decide about this, genbki.pl also needs the same treatment.

....Robert

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Alex Hunsaker on 20 Jan 2010 01:02
On Tue, Jan 19, 2010 at 13:06, Robert Haas <robertmhaas(a)gmail.com> wrote:
> On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(a)gmail.com> wrote:
>> ***************
>> *** 152,158 **** CATALOG(pg_attribute,1249) BKI_BOOTSTRAP
>> BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
>>        aclitem         attacl[1];
>>
>>        /* Column-level options */
>> !       aclitem         attoptions[1];
>>  } FormData_pg_attribute;
>>
>>  /*
>> --- 152,158 ----
>>        aclitem         attacl[1];
>>
>>        /* Column-level options */
>> !       text            attoptions[1];
>>  } FormData_pg_attribute;
>>
>>  /*
>
> Unfortunately this change (which is obviously correct and necessary)
> breaks the build on src/backend/catalog/heap.c with:
>
> heap.c:122: error: missing braces around initializer
> heap.c:122: error: (near initialization for ‘a1.attoptions[0]’)

Huh. I must have not done an --enable-depend build :( Even so after
a make clean I just get warnings...
This is with gcc version 4.4.2 20091208 (prerelease) (GCC)

Well for grins I tried changing it to the obvious {{{0}, {0}}} as the
places that seem to use it mark it as "null" anyway (see heap.c
InsertPgAttributeTupe ~525).

But then relcache.c gets more warnings:
relcache.c:87: warning: missing braces around initialize
relcache.c:87: warning: (near initialization for
‘Desc_pg_class[0].attoptions[0]’)
relcache.c:88: warning: missing braces around initialize
....

Which comes from genbki.pl. Seems we are stuck with:
1) the non portable
$row->{attoptions} = q|.attoptions = { {0}, {0} }|;

2) just dont initialize as nothing seems need it (*note* I have not
looked very hard)
$row->{attoptions} = q||;

Thoughts? Id rather not have this useful patch fall on the floor
because of a stupid limitation like this :)

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Alex Hunsaker on 20 Jan 2010 01:08
On Tue, Jan 19, 2010 at 23:02, Alex Hunsaker <badalex(a)gmail.com> wrote:
> On Tue, Jan 19, 2010 at 13:06, Robert Haas <robertmhaas(a)gmail.com> wrote:
>> On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(a)gmail.com> wrote:
>>> !       text            attoptions[1];

>> Unfortunately this change (which is obviously correct and necessary)
>> breaks the build on src/backend/catalog/heap.c with:

> 2) just dont initialize as nothing seems need it (*note* I have not
> looked very hard)
> $row->{attoptions}  = q||;

This should be OK because it will be zeroed anyway because its in the
..bss right?

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Alex Hunsaker on 21 Jan 2010 00:57
On Wed, Jan 20, 2010 at 19:51, Robert Haas <robertmhaas(a)gmail.com> wrote:
> On Tue, Jan 19, 2010 at 10:51 AM, Alex Hunsaker <badalex(a)gmail.com> wrote:
>> But yes, lets keep it simple for now.
>
> OK.  Updated patch attached.  Changes:
>
> - Incorporate your previous review patch.
> - Omit attacl and attoptions from hardcoded relation descriptor
> initializers so the whole thing still builds.

Seems to me a comment about the above might be nice. Something like
/* Things after here are should always be default null */ in
pg_attribute.h ?

Other than the below it looks good to me.

*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 2426,2437 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
ATSimplePermissionsRelationOrIndex(rel);
! ATSimpleRecursion(wqueue, rel, cmd, recurse);
pass = AT_PASS_COL_ATTRS;
break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
ATSimplePermissions(rel, false);
! /* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_COL_ATTRS;
break;
--- 2426,2437 ----
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
ATSimplePermissionsRelationOrIndex(rel);
! /* This command never recurses */
pass = AT_PASS_COL_ATTRS;
break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
ATSimplePermissions(rel, false);
! ATSimpleRecursion(wqueue, rel, cmd, recurse);
/* No command-specific prep needed */
pass = AT_PASS_COL_ATTRS;
break;

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 21 Jan 2010 09:30
On Thu, Jan 21, 2010 at 12:57 AM, Alex Hunsaker <badalex(a)gmail.com> wrote:
> Seems to me a comment about the above might be nice.  Something like
> /* Things after here are should always be default null */ in
> pg_attribute.h ?

Well... that wouldn't actually be a correct summary, so no. The point
is that variable-length fields are not used in tuple descriptors.
I'll add a comment about that.

> Other than the below it looks good to me.

Oh, dear. OK, I'll fix that.

....Robert

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: PG_MODULE_MAGIC checks and pg_migrator
Next: Streaming replication, loose ends