Обсуждение: Re: [HACKERS] Re: varchar() troubles (fwd)

Forwarded message:
> > Why attlen should be -1 ?
> > attlen in pg_attribute for v in table t is 84, why run-time attlen
> > should be -1 ? How else maxlen constraint could be checked ?
> > IMHO, you have to change heap_getattr() to check is atttype == VARCHAROID
> > and use vl_len if yes. Also, other places where attlen is used must be
> > changed too - e.g. ExecEvalVar():
> >
> >     {
> >         len = tuple_type->attrs[attnum - 1]->attlen;
> >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >         byval = tuple_type->attrs[attnum - 1]->attbyval ? true : false;
> >     }
> >
> >     execConstByVal = byval;
> >     execConstLen = len;
> >     ^^^^^^^^^^^^^^^^^^ - used in nodeHash.c
> >
>
> The major problem is that TupleDesc comes from several places, and
> attlen means several things.
>
> There are some cases where TupleDesc (int numatt, Attrs[]) is created
> on-the-fly (tupdesc.c), and the attlen is the length of the type.  In
> other cases, we get attlen from opening the relation, heap_open(), and
> in these cases it is the length as defined for the particular attribute.
>
> Certainly a bad situation.  I am not sure about a fix.

OK, here is a temporary fix to the problem.  It does the heap_open(),
then replaces the attrs for VARCHAR with attlen of -1.  You can't just
change the field, because the data is in a cache and you are just
returned a pointer.

Can I add an 'attdeflen' for "attributed defined length" field to
pg_attribute, and change the attlen references needed to the new field?
This is the only proper way to fix it.


---------------------------------------------------------------------------

*** ./backend/executor/execAmi.c.orig    Thu Jan 15 22:42:13 1998
--- ./backend/executor/execAmi.c    Thu Jan 15 23:54:37 1998
***************
*** 42,47 ****
--- 42,48 ----
  #include "access/genam.h"
  #include "access/heapam.h"
  #include "catalog/heap.h"
+ #include "catalog/pg_type.h"

  static Pointer
  ExecBeginScan(Relation relation, int nkeys, ScanKey skeys,
***************
*** 124,129 ****
--- 125,155 ----
      if (relation == NULL)
          elog(DEBUG, "ExecOpenR: relation == NULL, heap_open failed.");

+     {
+         int i;
+         Relation trel = palloc(sizeof(RelationData));
+         TupleDesc tdesc = palloc(sizeof(struct tupleDesc));
+         AttributeTupleForm *tatt =
+                 palloc(sizeof(AttributeTupleForm*)*relation->rd_att->natts);
+
+         memcpy(trel, relation, sizeof(RelationData));
+         memcpy(tdesc, relation->rd_att, sizeof(struct tupleDesc));
+         trel->rd_att = tdesc;
+         tdesc->attrs = tatt;
+
+         for (i = 0; i < relation->rd_att->natts; i++)
+         {
+             if (relation->rd_att->attrs[i]->atttypid != VARCHAROID)
+                 tdesc->attrs[i] = relation->rd_att->attrs[i];
+             else
+             {
+                 tdesc->attrs[i] = palloc(sizeof(FormData_pg_attribute));
+                 memcpy(tdesc->attrs[i], relation->rd_att->attrs[i],
+                                             sizeof(FormData_pg_attribute));
+                 tdesc->attrs[i]->attlen = -1;
+             }
+         }
+     }
      return relation;
  }



--
Bruce Momjian
maillist@candle.pha.pa.us

> OK, here is a temporary fix to the problem.  It does the heap_open(),
> then replaces the attrs for VARCHAR with attlen of -1.  You can't just
> change the field, because the data is in a cache and you are just
> returned a pointer.
>
> Can I add an 'attdeflen' for "attributed defined length" field to
> pg_attribute, and change the attlen references needed to the new field?
> This is the only proper way to fix it.

Bruce, does your "temporary fix" seem to repair all known problems with varchar()? If so, would you be interested in
holding off on a "proper fix" and coming back to it after v6.3 is released? At that time, we can try solving the
general
problem of retaining column-specific attributes, such as your max len for varchar, declared dimensions for arrays, and
numeric() and decimal() types. Or, if you have time to try a solution now _and_ come back to it later...

                                                      - Tom

>
> > OK, here is a temporary fix to the problem.  It does the heap_open(),
> > then replaces the attrs for VARCHAR with attlen of -1.  You can't just
> > change the field, because the data is in a cache and you are just
> > returned a pointer.
> >
> > Can I add an 'attdeflen' for "attributed defined length" field to
> > pg_attribute, and change the attlen references needed to the new field?
> > This is the only proper way to fix it.
>
> Bruce, does your "temporary fix" seem to repair all known problems with varchar()? If so, would you be interested in
> holding off on a "proper fix" and coming back to it after v6.3 is released? At that time, we can try solving the
general
> problem of retaining column-specific attributes, such as your max len for varchar, declared dimensions for arrays,
and
> numeric() and decimal() types. Or, if you have time to try a solution now _and_ come back to it later...
>
>                                                       - Tom
>
>

In fact, I am inclined to leave attlen unchanged, and add atttyplen that
is a copy of the length of the type.  That way, the attlen for varchar()
really contains the defined length, and atttyplen is used for disk
references, and it is very clear what it means.


--
Bruce Momjian
maillist@candle.pha.pa.us

>
> > OK, here is a temporary fix to the problem.  It does the heap_open(),
> > then replaces the attrs for VARCHAR with attlen of -1.  You can't just
> > change the field, because the data is in a cache and you are just
> > returned a pointer.
> >
> > Can I add an 'attdeflen' for "attributed defined length" field to
> > pg_attribute, and change the attlen references needed to the new field?
> > This is the only proper way to fix it.
>

> Bruce, does your "temporary fix" seem to repair all known problems
with varchar()? If so, would you be interested in > holding off on a
"proper fix" and coming back to it after v6.3 is released? At that time,
we can try solving the general > problem of retaining column-specific
attributes, such as your max len for varchar, declared dimensions for
arrays, and > numeric() and decimal() types. Or, if you have time to try
a solution now _and_ come back to it later... >  >

[Those wide post really are difficult.]

I don't think my solution is perfect or complete.  I only caught one
group of heap_open calls used in the executor.  I could funnel all of
them through this patched function, but I can imagine there would be
ones I would miss.  Once the structure is gotten from the cache, it
seems to fly around the executor code quite freely, and it is hard to
know when a tuple descriptor is being created, if it is being used for
data creation or data reference.  attlen references are much clearer in
their intent.

If I add a new field type to FormData_pg_attribute, I can then check
each attlen reference, and check if it is trying to move through the
on-disk storage (attlen/typlen) or create a new/modify an entry
(attdeflen).

How much time I have depends on what Vadim needs me to do for
subselects.


--
Bruce Momjian
maillist@candle.pha.pa.us

Bruce Momjian wrote:
>
> > >
> > > Can I add an 'attdeflen' for "attributed defined length" field to
> > > pg_attribute, and change the attlen references needed to the new field?
> > > This is the only proper way to fix it.
> >
> > Bruce, does your "temporary fix" seem to repair all known problems with varchar()? If so, would you be interested
in
> > holding off on a "proper fix" and coming back to it after v6.3 is released? At that time, we can try solving the
general
> > problem of retaining column-specific attributes, such as your max len for varchar, declared dimensions for arrays,
and
> > numeric() and decimal() types. Or, if you have time to try a solution now _and_ come back to it later...
> >
>
> In fact, I am inclined to leave attlen unchanged, and add atttyplen that
> is a copy of the length of the type.  That way, the attlen for varchar()
> really contains the defined length, and atttyplen is used for disk
> references, and it is very clear what it means.

pg_attribute.h:

    int2        attlen;

    /*
     * attlen is a copy of the typlen field from pg_type for this
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     * attribute...
       ^^^^^^^^^
: I would suggest to don't change this to preserve the same meaning
for all data types. attlen = -1 means that attribute is varlena.

We certainly need in new field in pg_attribute! I don't like names like
"attdeflen" or "atttyplen" - bad names for NUMERIC etc. Something like
atttspec (ATTribute Type SPECification) is better, imho.

For the varchar(N) we'll have attlen = -1 and atttspec = N (or N + 4 - what's
better). For the text: attlen = -1 and atttspec = -1. And so on.

Of 'course, it's not so much matter where to put maxlen of varchar.
attlen = -1 for varchar just seems more clear to me.

But in any case we need in new field and, imho, this should be added
in 6.3

Vadim

>
> Bruce Momjian wrote:
> >
> > > >
> > > > Can I add an 'attdeflen' for "attributed defined length" field to
> > > > pg_attribute, and change the attlen references needed to the new field?
> > > > This is the only proper way to fix it.
> > >
> > > Bruce, does your "temporary fix" seem to repair all known problems with varchar()? If so, would you be interested
in
> > > holding off on a "proper fix" and coming back to it after v6.3 is released? At that time, we can try solving the
general
> > > problem of retaining column-specific attributes, such as your max len for varchar, declared dimensions for
arrays,and 
> > > numeric() and decimal() types. Or, if you have time to try a solution now _and_ come back to it later...
> > >
> >
> > In fact, I am inclined to leave attlen unchanged, and add atttyplen that
> > is a copy of the length of the type.  That way, the attlen for varchar()
> > really contains the defined length, and atttyplen is used for disk
> > references, and it is very clear what it means.
>
> pg_attribute.h:
>
>     int2        attlen;
>
>     /*
>      * attlen is a copy of the typlen field from pg_type for this
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      * attribute...
>        ^^^^^^^^^
> : I would suggest to don't change this to preserve the same meaning
> for all data types. attlen = -1 means that attribute is varlena.
>
> We certainly need in new field in pg_attribute! I don't like names like
> "attdeflen" or "atttyplen" - bad names for NUMERIC etc. Something like
> atttspec (ATTribute Type SPECification) is better, imho.
>
> For the varchar(N) we'll have attlen = -1 and atttspec = N (or N + 4 - what's
> better). For the text: attlen = -1 and atttspec = -1. And so on.
>
> Of 'course, it's not so much matter where to put maxlen of varchar.
> attlen = -1 for varchar just seems more clear to me.
>
> But in any case we need in new field and, imho, this should be added
> in 6.3

OK, we have a new pg_attribute column called 'atttypmod' for
'type-specific modifier'.  Currently, it is only used to hold the char
and varchar length, but I am sure will be used soon for other types.

Here is the test:

    test=> insert into test values ('asdfasdfasdfasdfasdfadsfasdf11',3);
    INSERT 18282 1
    test=> select * from test;
    x                   |y
    --------------------+-
    asdfasdfasdfasdfasdf|3
    (1 row)

'attlen' was certainly a confusing double-used field that I am glad to
return to single-use status.

I will be installing the patch soon, and will then start on subselects
in the parser.  It will probably take me until Monday to finish that.

--
Bruce Momjian
maillist@candle.pha.pa.us

Bruce Momjian wrote:
>
> OK, we have a new pg_attribute column called 'atttypmod' for
> 'type-specific modifier'.  Currently, it is only used to hold the char
> and varchar length, but I am sure will be used soon for other types.

Nice!

>
> Here is the test:
>
>         test=> insert into test values ('asdfasdfasdfasdfasdfadsfasdf11',3);
>         INSERT 18282 1
>         test=> select * from test;
>         x                   |y
>         --------------------+-
>         asdfasdfasdfasdfasdf|3
>         (1 row)
>
> 'attlen' was certainly a confusing double-used field that I am glad to
> return to single-use status.
>
> I will be installing the patch soon, and will then start on subselects
> in the parser.  It will probably take me until Monday to finish that.

Ok.

Vadim