dblink_build_sql_update versus dropped columns
A recent bug report
http://archives.postgresql.org/pgsql-admin/2010-06/msg00101.php
shows that dblink_build_sql_update and friends are really not all there
when it comes to dealing with dropped columns in the target table.
The immediate cause of the reported crash is just an internal matter,
but while looking at it I realized that there is also an API issue:
are the column numbers in the passed-in primary_key_attnums array to be
taken as logical or physical attnums? If the user extracted the array
from a pg_index entry then they are physical attnums, but if he just
writes the array by hand then they are probably logical numbers, ie,
they would not count any dropped columns appearing before the PK
columns.
I suspect the point has never come up before because PKs are commonly
the first columns anyway.
The current effective behavior of the code is that the column numbers
are physical numbers. Should we document it that way, or change it?
regards, tom lane
On 06/14/2010 10:58 AM, Tom Lane wrote:
A recent bug report
http://archives.postgresql.org/pgsql-admin/2010-06/msg00101.php
shows that dblink_build_sql_update and friends are really not all there
when it comes to dealing with dropped columns in the target table.
Yup, was just looking at that...
The immediate cause of the reported crash is just an internal matter,
but while looking at it I realized that there is also an API issue:
are the column numbers in the passed-in primary_key_attnums array to be
taken as logical or physical attnums? If the user extracted the array
from a pg_index entry then they are physical attnums, but if he just
writes the array by hand then they are probably logical numbers, ie,
they would not count any dropped columns appearing before the PK
columns.
Yes, it uses physical attnums, mainly because it was originally written
before we even supported dropped columns and never changed/fixed it.
I suspect the point has never come up before because PKs are commonly
the first columns anyway.The current effective behavior of the code is that the column numbers
are physical numbers. Should we document it that way, or change it?
Probably it should be changed to deal with dropped columns correctly,
but I won't have time to look at this closely until the end of the month
-- is that soon enough?
Thanks,
Joe
Joe Conway <mail@joeconway.com> writes:
On 06/14/2010 10:58 AM, Tom Lane wrote:
The current effective behavior of the code is that the column numbers
are physical numbers. Should we document it that way, or change it?
Probably it should be changed to deal with dropped columns correctly,
but I won't have time to look at this closely until the end of the month
-- is that soon enough?
Actually, I was working on it myself. On further reflection I think
that logical numbers are clearly the right thing --- if we define it
as being physical numbers then we will have headaches in the future
when/if we support rearranging columns. However, there is some small
chance of breaking things in existing DBs if we back-patch that change.
Thoughts?
It strikes me also that the code is not nearly careful enough about
defending itself against garbage input in the primary_key_attnums
argument ...
regards, tom lane
On 06/14/2010 11:21 AM, Tom Lane wrote:
Actually, I was working on it myself. On further reflection I think
that logical numbers are clearly the right thing --- if we define it
as being physical numbers then we will have headaches in the future
when/if we support rearranging columns. However, there is some small
chance of breaking things in existing DBs if we back-patch that change.
Thoughts?
I didn't even think people were using those functions for many years
since I never heard any complaints. I'd say better to not backpatch
changes to logical ordering, but FWIW the attached at least fixes the
immediate bug in head and ought to work at least a few branches.
It strikes me also that the code is not nearly careful enough about
defending itself against garbage input in the primary_key_attnums
argument ...
Probably not :-(
Joe
Attachments:
dblink.2010.06.14.01.difftext/x-patch; name=dblink.2010.06.14.01.diffDownload
Index: dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.94
diff -c -r1.94 dblink.c
*** dblink.c 9 Jun 2010 03:39:26 -0000 1.94
--- dblink.c 14 Jun 2010 18:37:07 -0000
***************
*** 94,100 ****
static char *quote_literal_cstr(char *rawstr);
static char *quote_ident_cstr(char *rawstr);
static int16 get_attnum_pk_pos(int2vector *pkattnums, int16 pknumatts, int16 key);
! static HeapTuple get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals);
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
static void dblink_connstr_check(const char *connstr);
--- 94,100 ----
static char *quote_literal_cstr(char *rawstr);
static char *quote_ident_cstr(char *rawstr);
static int16 get_attnum_pk_pos(int2vector *pkattnums, int16 pknumatts, int16 key);
! static HeapTuple get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals, TupleDesc *reltupdesc);
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
static void dblink_connstr_check(const char *connstr);
***************
*** 1768,1774 ****
static char *
get_sql_insert(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals, char **tgt_pkattvals)
{
- Relation rel;
char *relname;
HeapTuple tuple;
TupleDesc tupdesc;
--- 1768,1773 ----
***************
*** 1784,1801 ****
/* get relation name including any needed schema prefix and quoting */
relname = generate_relation_name(relid);
! /*
! * Open relation using relid
! */
! rel = relation_open(relid, AccessShareLock);
! tupdesc = rel->rd_att;
! natts = tupdesc->natts;
!
! tuple = get_tuple_of_interest(relid, pkattnums, pknumatts, src_pkattvals);
if (!tuple)
ereport(ERROR,
(errcode(ERRCODE_CARDINALITY_VIOLATION),
errmsg("source row not found")));
appendStringInfo(&buf, "INSERT INTO %s(", relname);
--- 1783,1794 ----
/* get relation name including any needed schema prefix and quoting */
relname = generate_relation_name(relid);
! tuple = get_tuple_of_interest(relid, pkattnums, pknumatts, src_pkattvals, &tupdesc);
if (!tuple)
ereport(ERROR,
(errcode(ERRCODE_CARDINALITY_VIOLATION),
errmsg("source row not found")));
+ natts = tupdesc->natts;
appendStringInfo(&buf, "INSERT INTO %s(", relname);
***************
*** 1848,1854 ****
}
appendStringInfo(&buf, ")");
- relation_close(rel, AccessShareLock);
return (buf.data);
}
--- 1841,1846 ----
***************
*** 1903,1909 ****
static char *
get_sql_update(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals, char **tgt_pkattvals)
{
- Relation rel;
char *relname;
HeapTuple tuple;
TupleDesc tupdesc;
--- 1895,1900 ----
***************
*** 1919,1936 ****
/* get relation name including any needed schema prefix and quoting */
relname = generate_relation_name(relid);
! /*
! * Open relation using relid
! */
! rel = relation_open(relid, AccessShareLock);
! tupdesc = rel->rd_att;
! natts = tupdesc->natts;
!
! tuple = get_tuple_of_interest(relid, pkattnums, pknumatts, src_pkattvals);
if (!tuple)
ereport(ERROR,
(errcode(ERRCODE_CARDINALITY_VIOLATION),
errmsg("source row not found")));
appendStringInfo(&buf, "UPDATE %s SET ", relname);
--- 1910,1921 ----
/* get relation name including any needed schema prefix and quoting */
relname = generate_relation_name(relid);
! tuple = get_tuple_of_interest(relid, pkattnums, pknumatts, src_pkattvals, &tupdesc);
if (!tuple)
ereport(ERROR,
(errcode(ERRCODE_CARDINALITY_VIOLATION),
errmsg("source row not found")));
+ natts = tupdesc->natts;
appendStringInfo(&buf, "UPDATE %s SET ", relname);
***************
*** 1992,1998 ****
appendStringInfo(&buf, " IS NULL");
}
- relation_close(rel, AccessShareLock);
return (buf.data);
}
--- 1977,1982 ----
***************
*** 2050,2064 ****
}
static HeapTuple
! get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals)
{
Relation rel;
char *relname;
- TupleDesc tupdesc;
StringInfoData buf;
int ret;
HeapTuple tuple;
int i;
initStringInfo(&buf);
--- 2034,2049 ----
}
static HeapTuple
! get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals, TupleDesc *tupdesc)
{
Relation rel;
char *relname;
StringInfoData buf;
int ret;
+ TupleDesc local_tupdesc;
HeapTuple tuple;
int i;
+ MemoryContext caller_cxt = CurrentMemoryContext;
initStringInfo(&buf);
***************
*** 2069,2075 ****
* Open relation using relid
*/
rel = relation_open(relid, AccessShareLock);
! tupdesc = CreateTupleDescCopy(rel->rd_att);
relation_close(rel, AccessShareLock);
/*
--- 2054,2060 ----
* Open relation using relid
*/
rel = relation_open(relid, AccessShareLock);
! local_tupdesc = CreateTupleDescCopy(rel->rd_att);
relation_close(rel, AccessShareLock);
/*
***************
*** 2093,2099 ****
appendStringInfo(&buf, " AND ");
appendStringInfoString(&buf,
! quote_ident_cstr(NameStr(tupdesc->attrs[pkattnum - 1]->attname)));
if (src_pkattvals[i] != NULL)
appendStringInfo(&buf, " = %s",
--- 2078,2084 ----
appendStringInfo(&buf, " AND ");
appendStringInfoString(&buf,
! quote_ident_cstr(NameStr(local_tupdesc->attrs[pkattnum - 1]->attname)));
if (src_pkattvals[i] != NULL)
appendStringInfo(&buf, " = %s",
***************
*** 2118,2124 ****
else if (ret == SPI_OK_SELECT && SPI_processed == 1)
{
! SPITupleTable *tuptable = SPI_tuptable;
tuple = SPI_copytuple(tuptable->vals[0]);
SPI_finish();
--- 2103,2117 ----
else if (ret == SPI_OK_SELECT && SPI_processed == 1)
{
! SPITupleTable *tuptable = SPI_tuptable;
! TupleDesc spi_tupdesc;
! MemoryContext oldcxt;
!
! oldcxt = MemoryContextSwitchTo(caller_cxt);
! spi_tupdesc = CreateTupleDescCopy(tuptable->tupdesc);
! MemoryContextSwitchTo(oldcxt);
!
! *tupdesc = spi_tupdesc;
tuple = SPI_copytuple(tuptable->vals[0]);
SPI_finish();
Joe Conway <mail@joeconway.com> writes:
I didn't even think people were using those functions for many years
since I never heard any complaints. I'd say better to not backpatch
changes to logical ordering, but FWIW the attached at least fixes the
immediate bug in head and ought to work at least a few branches.
[squint...] This doesn't appear to me to fix the problem. You really
need the query-construction loops to track logical and physical numbers
separately.
regards, tom lane
On 06/14/2010 11:54 AM, Tom Lane wrote:
Joe Conway <mail@joeconway.com> writes:
I didn't even think people were using those functions for many years
since I never heard any complaints. I'd say better to not backpatch
changes to logical ordering, but FWIW the attached at least fixes the
immediate bug in head and ought to work at least a few branches.[squint...] This doesn't appear to me to fix the problem. You really
need the query-construction loops to track logical and physical numbers
separately.
Hmmm, worked for the provided case, but this is a good example of why I
*usually* don't send a patch to the list without spending more quality
time thinking about the problem ;-)
Joe