Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
Peter Eisentraut writes: > On mån, 2012-01-09 at 14:04 -0500, Tom Lane wrote: >> #ifdef CATALOG_VARLEN /* variable-length fields start here >> */ >> >> to be even clearer. >> >> What would be appropriate to add instead of those inconsistently-used >> comments is explicit comments about the exception cases such as >> proargtypes, to make it clear that the placement of the #ifdef >> CATALOG_VARLEN is intentional and not a bug in those cases. > I implemented your suggestions in the attached patch. This looks ready to go to me, except for one trivial nit: in pg_extension.h, please keep the comment pointing out that extversion should never be null. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
On mån, 2012-01-09 at 14:04 -0500, Tom Lane wrote:
> #ifdef CATALOG_VARLEN /* variable-length fields start here
> */
>
> to be even clearer.
>
> What would be appropriate to add instead of those inconsistently-used
> comments is explicit comments about the exception cases such as
> proargtypes, to make it clear that the placement of the #ifdef
> CATALOG_VARLEN is intentional and not a bug in those cases.
>
I implemented your suggestions in the attached patch.
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 1ba935c..cdb0bee 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -143,6 +143,7 @@ sub Catalogs
elsif ($declaring_attributes)
{
next if (/^{|^$/);
+next if (/^#/);
if (/^}/)
{
undef $declaring_attributes;
@@ -150,6 +151,7 @@ sub Catalogs
else
{
my ($atttype, $attname) = split /\s+/, $_;
+die "parse error ($input_file)" unless $attname;
if (exists $RENAME_ATTTYPE{$atttype})
{
$atttype = $RENAME_ATTTYPE{$atttype};
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index d0138fc..a59950e 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3351,15 +3351,30 @@ RelationGetIndexList(Relation relation)
while (HeapTupleIsValid(htup = systable_getnext(indscan)))
{
Form_pg_index index = (Form_pg_index) GETSTRUCT(htup);
+ Datum indclassDatum;
+ oidvector *indclass;
+ bool isnull;
/* Add index's OID to result list in the proper order */
result = insert_ordered_oid(result, index->indexrelid);
+ /*
+ * indclass cannot be referenced directly through the C struct, because
+ * it comes after the variable-width indkey field. Must extract the
+ * datum the hard way...
+ */
+ indclassDatum = heap_getattr(htup,
+ Anum_pg_index_indclass,
+ GetPgIndexDescriptor(),
+ &isnull);
+ Assert(!isnull);
+ indclass = (oidvector *) DatumGetPointer(indclassDatum);
+
/* Check to see if it is a unique, non-partial btree index on OID */
if (index->indnatts == 1 &&
index->indisunique && index->indimmediate &&
index->indkey.values[0] == ObjectIdAttributeNumber &&
- index->indclass.values[0] == OID_BTREE_OPS_OID &&
+ indclass->values[0] == OID_BTREE_OPS_OID &&
heap_attisnull(htup, Anum_pg_index_indpred))
oidIndex = index->indexrelid;
}
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 5fe3a5b..bcf31e6 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -22,6 +22,16 @@
/* Introduces a catalog's structure definition */
#define CATALOG(name,oid) typedef struct CppConcat(FormData_,name)
+/*
+ * This is never defined; it's here only for documentation.
+ *
+ * Variable-length catalog fields (except possibly the first not nullable one)
+ * should not be visible in C structures, so they are made invisible by #ifdefs
+ * of an undefined symbol. See also MARKNOTNULL in bootstrap.c for how this is
+ * handled.
+ */
+#undef CATALOG_VARLEN
+
/* Options that may appear after CATALOG (on the same line) */
#define BKI_BOOTSTRAP
#define BKI_SHARED_RELATION
diff --git a/src/include/catalog/pg_aggregate.h b/src/include/catalog/pg_aggregate.h
index 4e10021..0c8a20c 100644
--- a/src/include/catalog/pg_aggregate.h
+++ b/src/include/catalog/pg_aggregate.h
@@ -44,7 +44,9 @@ CATALOG(pg_aggregate,2600) BKI_WITHOUT_OIDS
regproc aggfinalfn;
Oid aggsortop;
Oid aggtranstype;
- text agginitval; /* VARIABLE LENGTH FIELD */
+#ifdef CATALOG_VARLEN /* variable-length fields start here */
+ text agginitval;
+#endif
} FormData_pg_aggregate;
/*
diff --git a/src/include/catalog/pg_attrdef.h b/src/include/catalog/pg_attrdef.h
index 01f663c..ad770e4 100644
--- a/src/include/catalog/pg_attrdef.h
+++ b/src/include/catalog/pg_attrdef.h
@@ -32,8 +32,10 @@ CATALOG(pg_attrdef,2604)
{
Oid adrelid; /* OID of table containing attribute */
int2 adnum; /* attnum of attribute */
+#ifdef CATALOG_VARLEN /* variable-length fields start here */
pg_node_tree adbin; /* nodeToString representation of default */
text adsrc; /* human-readable representation of default */
+#endif
} FormData_pg_attrdef;
/*
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 5cb16f6..45e38e4 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -145,11 +145,8 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
/* attribute's collation */
Oid attcollation;
- /*
- * VARIABLE LENGTH FIELDS start here. These fields may be NULL, too.
- *
- * NOTE: the following fields are no
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
Peter Eisentraut writes: > So I think the relcache.c thing should be fixed and then this might be > good to go. Cosmetic gripes: I think we could get rid of the various comments that say things like "variable length fields start here", since the #ifdef CATALOG_VARLEN lines now represent that in a standardized fashion. Possibly those lines should be #ifdef CATALOG_VARLEN /* variable-length fields start here */ to be even clearer. What would be appropriate to add instead of those inconsistently-used comments is explicit comments about the exception cases such as proargtypes, to make it clear that the placement of the #ifdef CATALOG_VARLEN is intentional and not a bug in those cases. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
So here is a patch for that.
There are a few cases that break when hiding all variable length fields:
Access to indclass in relcache.c, as discussed upthread, which should be
fixed.
Access to pg_largeobject.data. This is apparently OK, per comment in
inv_api.c.
Access to pg_proc.proargtypes in various places. This is clearly
useful, so we'll keep it visible.
So I think the relcache.c thing should be fixed and then this might be
good to go.
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 1ba935c..1b8db1b 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -143,6 +143,7 @@ sub Catalogs
elsif ($declaring_attributes)
{
next if (/^{|^$/);
+next if (/^#/);
if (/^}/)
{
undef $declaring_attributes;
@@ -150,6 +151,7 @@ sub Catalogs
else
{
my ($atttype, $attname) = split /\s+/, $_;
+die unless $attname;
if (exists $RENAME_ATTTYPE{$atttype})
{
$atttype = $RENAME_ATTTYPE{$atttype};
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 5fe3a5b..bcf31e6 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -22,6 +22,16 @@
/* Introduces a catalog's structure definition */
#define CATALOG(name,oid) typedef struct CppConcat(FormData_,name)
+/*
+ * This is never defined; it's here only for documentation.
+ *
+ * Variable-length catalog fields (except possibly the first not nullable one)
+ * should not be visible in C structures, so they are made invisible by #ifdefs
+ * of an undefined symbol. See also MARKNOTNULL in bootstrap.c for how this is
+ * handled.
+ */
+#undef CATALOG_VARLEN
+
/* Options that may appear after CATALOG (on the same line) */
#define BKI_BOOTSTRAP
#define BKI_SHARED_RELATION
diff --git a/src/include/catalog/pg_aggregate.h b/src/include/catalog/pg_aggregate.h
index 4e10021..c420bdb 100644
--- a/src/include/catalog/pg_aggregate.h
+++ b/src/include/catalog/pg_aggregate.h
@@ -44,7 +44,9 @@ CATALOG(pg_aggregate,2600) BKI_WITHOUT_OIDS
regproc aggfinalfn;
Oid aggsortop;
Oid aggtranstype;
+#ifdef CATALOG_VARLEN
text agginitval; /* VARIABLE LENGTH FIELD */
+#endif
} FormData_pg_aggregate;
/*
diff --git a/src/include/catalog/pg_attrdef.h b/src/include/catalog/pg_attrdef.h
index 01f663c..ee205ec 100644
--- a/src/include/catalog/pg_attrdef.h
+++ b/src/include/catalog/pg_attrdef.h
@@ -32,8 +32,10 @@ CATALOG(pg_attrdef,2604)
{
Oid adrelid; /* OID of table containing attribute */
int2 adnum; /* attnum of attribute */
+#ifdef CATALOG_VARLEN
pg_node_tree adbin; /* nodeToString representation of default */
text adsrc; /* human-readable representation of default */
+#endif
} FormData_pg_attrdef;
/*
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 5cb16f6..7d6d0ac 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -151,6 +151,7 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
* NOTE: the following fields are not present in tuple descriptors!
*/
+#ifdef CATALOG_VARLEN
/* Column-level access permissions */
aclitem attacl[1];
@@ -159,6 +160,7 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
/* Column-level FDW options */
text attfdwoptions[1];
+#endif
} FormData_pg_attribute;
/*
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 829f9b9..97a51d3 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -74,8 +74,10 @@ CATALOG(pg_class,1259) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83) BKI_SCHEMA_MACRO
* NOTE: these fields are not present in a relcache entry's rd_rel field.
*/
+#ifdef CATALOG_VARLEN
aclitem relacl[1]; /* access permissions */
text reloptions[1]; /* access-method-specific options */
+#endif
} FormData_pg_class;
/* Size of fixed part of pg_class tuples, not counting var-length fields */
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index c962834..91bcc54 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -130,6 +130,7 @@ CATALOG(pg_constraint,2606)
*/
Oid conexclop[1];
+#ifdef CATALOG_VARLEN
/*
* If a check constraint, nodeToString representation of expression
*/
@@ -139,6 +140,7 @@ CATALOG(pg_constraint,2606)
* If a check constraint, source-text representation of expression
*/
text consrc;
+#endif
} FormData_pg_constraint;
/*
diff --git a/src/include/catalog/pg_database.h b/src/include/catalog/pg_database.h
index 57537c8..3875d3f 100644
--- a/src/include/catalog/pg_database.h
+++ b/s
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
On Mon, Dec 5, 2011 at 3:12 PM, Tom Lane wrote: > Robert Haas writes: >> On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane wrote: >>> Peter Eisentraut writes: To clarify, I believe the rule is that the first variable-length field can be accessed as a struct field. Are there any exceptions to this? > >>> If it is known not null, yes, but I wonder just how many places actually >>> depend on that. > >> My impression is that all the varlena fields also allow nulls. > > See MARKNOTNULL in bootstrap.c. I think the exceptions were designed to > protect direct accesses to pg_index. Hmm, OK. rhaas=# select r.relname, a.attname, a.atttypid::regtype from pg_class r, pg_attribute a where relnamespace=11 and relkind='r' and attrelid = r.oid and a.attnotnull and a.attlen<0; relname | attname| atttypid +--+ pg_proc| proargtypes | oidvector pg_index | indkey | int2vector pg_index | indcollation | oidvector pg_index | indclass | oidvector pg_index | indoption| int2vector pg_trigger | tgattr | int2vector (6 rows) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
Robert Haas writes: > On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane wrote: >> Peter Eisentraut writes: >>> To clarify, I believe the rule is that the first variable-length field >>> can be accessed as a struct field. Are there any exceptions to this? >> If it is known not null, yes, but I wonder just how many places actually >> depend on that. > My impression is that all the varlena fields also allow nulls. See MARKNOTNULL in bootstrap.c. I think the exceptions were designed to protect direct accesses to pg_index. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane wrote: > Peter Eisentraut writes: >> To clarify, I believe the rule is that the first variable-length field >> can be accessed as a struct field. Are there any exceptions to this? > > If it is known not null, yes, but I wonder just how many places actually > depend on that. It might be better to remove all varlena fields from C > visibility and require use of the accessor functions. We should at > least look into what that would cost us. My impression is that all the varlena fields also allow nulls. So I think there's no point in trying to keep the first one C-accessible. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
Peter Eisentraut writes: > To clarify, I believe the rule is that the first variable-length field > can be accessed as a struct field. Are there any exceptions to this? If it is known not null, yes, but I wonder just how many places actually depend on that. It might be better to remove all varlena fields from C visibility and require use of the accessor functions. We should at least look into what that would cost us. > Also, this code in relcache.c accesses indclass, which is after an > int2vector and an oidvector field: > /* Check to see if it is a unique, non-partial btree index on OID */ > if (index->indnatts == 1 && > index->indisunique && index->indimmediate && > index->indkey.values[0] == ObjectIdAttributeNumber && > index->indclass.values[0] == OID_BTREE_OPS_OID && > heap_attisnull(htup, Anum_pg_index_indpred)) > oidIndex = index->indexrelid; Hmm, that does look mighty broken, doesn't it ... but somehow it works, else GetNewOid would be bleating all the time. (Thinks about that for a bit) Oh, it accidentally fails to fail because the C declarations for int2vector and oidvector are actually correct if there is a single element in the arrays, which we already verified with the indnatts test. But yeah, this seems horribly fragile, and it should not be performance critical because we only go through here when loading up a cache entry. So let's change it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
On sön, 2011-11-27 at 18:20 -0500, Tom Lane wrote:
> The low-tech way would be
>
> CATALOG(pg_attribute,1249) BKI_BOOTSTRAP ...
> {
> ...
> int4attinhcount;
> Oid attcollation;
> aclitem attacl[1];
> #ifdef CATALOG_VARLEN_FIELDS
> textattoptions[1];
> textattfdwoptions[1];
> #endif
> } FormData_pg_attribute;
Good enough.
To clarify, I believe the rule is that the first variable-length field
can be accessed as a struct field. Are there any exceptions to this?
This kind of comment is pretty confusing:
CATALOG(pg_rewrite,2618)
{
NameDatarulename;
Oid ev_class;
int2ev_attr;
charev_type;
charev_enabled;
boolis_instead;
/* NB: remaining fields must be accessed via heap_getattr */
pg_node_tree ev_qual;
pg_node_tree ev_action;
} FormData_pg_rewrite;
Also, this code in relcache.c accesses indclass, which is after an
int2vector and an oidvector field:
/* Check to see if it is a unique, non-partial btree index on OID */
if (index->indnatts == 1 &&
index->indisunique && index->indimmediate &&
index->indkey.values[0] == ObjectIdAttributeNumber &&
index->indclass.values[0] == OID_BTREE_OPS_OID &&
heap_attisnull(htup, Anum_pg_index_indpred))
oidIndex = index->indexrelid;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] hiding variable-length fields from Form_pg_* structs
Peter Eisentraut writes:
> CATALOG(pg_attribute,1249) BKI_BOOTSTRAP ...
> {
> ...
> int4attinhcount;
> Oid attcollation;
> aclitem attacl[1];
> CATVARLEN(
> textattoptions[1];
> textattfdwoptions[1];
> )
> } FormData_pg_attribute;
> where CATVARLEN is defined empty in C, and ignored in the BKI generation
> code.
> The real trick is to find something that handles well with pgindent and
> indenting text editors.
The low-tech way would be
CATALOG(pg_attribute,1249) BKI_BOOTSTRAP ...
{
...
int4attinhcount;
Oid attcollation;
aclitem attacl[1];
#ifdef CATALOG_VARLEN_FIELDS
textattoptions[1];
textattfdwoptions[1];
#endif
} FormData_pg_attribute;
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] hiding variable-length fields from Form_pg_* structs
It would be helpful if variable length catalog fields (except the first
one) would not be visible on the C level in the Form_pg_* structs. We
keep them listed in the include/catalog/pg_*.h files so that the BKI
generating code can see them and for general documentation, but the
fields are meaningless in C, and some catalog files contain free-form
comments advising the reader of that. If we could hide them somehow, we
would avoid random programming bugs, deconfuse compilers trying to
generate useful warnings, and save occasional stack space. There are
several known cases of the first and second issue, at least.
I haven't found the ideal way to implement that yet, but the general
idea would be something like:
CATALOG(pg_attribute,1249) BKI_BOOTSTRAP ...
{
...
int4attinhcount;
Oid attcollation;
aclitem attacl[1];
CATVARLEN(
textattoptions[1];
textattfdwoptions[1];
)
} FormData_pg_attribute;
where CATVARLEN is defined empty in C, and ignored in the BKI generation
code.
The real trick is to find something that handles well with pgindent and
indenting text editors.
Ideas?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
