Thread: several minor cleanups
The attached patch fixes some spelling mistakes, makes the comments on one of the optimizer functions a lot more clear, adds a summary of the recent KSQO discussion to the comments in the code, adds regression tests for the bug with sequence state Tom fixed recently and another reg. test, and removes some PostQuel legacy stuff: ExecAppend -> ExecInsert, ExecRetrieve -> ExecSelect, etc. This was changed because the elog() messages from this routine are user-visible, so we should be using the SQL terms. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> The attached patch fixes some spelling mistakes, makes the
> comments on one of the optimizer functions a lot more
> clear, adds a summary of the recent KSQO discussion to the
> comments in the code, adds regression tests for the bug with
> sequence state Tom fixed recently and another reg. test, and
> removes some PostQuel legacy stuff: ExecAppend -> ExecInsert,
> ExecRetrieve -> ExecSelect, etc. This was changed because the
> elog() messages from this routine are user-visible, so we
> should be using the SQL terms.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilconway@rogers.com>
> PGP Key ID: DB3C29FC
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Neil Conway <nconway@klamath.dyndns.org> writes:
***************
*** 251,257 ****
ExecCheckRTPerms(parseTree->rtable, operation);
/*
! * Search for subplans and APPEND nodes to check their rangetables.
*/
ExecCheckPlanPerms(plan, parseTree->rtable, operation);
}
--- 251,257 ----
ExecCheckRTPerms(parseTree->rtable, operation);
/*
! * Search for subplans and INSERT nodes to check their rangetables.
*/
ExecCheckPlanPerms(plan, parseTree->rtable, operation);
}
***************
This comment was right beforehand and is so no longer :-(.
Otherwise I'm okay with this, if we don't mind the probability of
breaking existing client applications that are looking for ExecAppend:
messages. One might think that unnecessary changes in common error
messages are not a good idea until sometime after we've implemented an
error code facility and given people a chance to move over to looking
at error codes instead of error strings.
regards, tom lane
On Mon, Jun 24, 2002 at 04:53:17PM -0400, Tom Lane wrote: > Neil Conway <nconway@klamath.dyndns.org> writes: > *************** > *** 251,257 **** > ExecCheckRTPerms(parseTree->rtable, operation); > > /* > ! * Search for subplans and APPEND nodes to check their rangetables. > */ > ExecCheckPlanPerms(plan, parseTree->rtable, operation); > } > --- 251,257 ---- > ExecCheckRTPerms(parseTree->rtable, operation); > > /* > ! * Search for subplans and INSERT nodes to check their rangetables. > */ > ExecCheckPlanPerms(plan, parseTree->rtable, operation); > } > *************** > > This comment was right beforehand and is so no longer :-(. Woops, s/append/insert/g was a bit over-enthusiastic. A revised patch is attached (it's also updated for CVS HEAD). > Otherwise I'm okay with this, if we don't mind the probability of > breaking existing client applications that are looking for ExecAppend: > messages. I would be skeptical of any client application that tries to divine information about the result of an operation by inspecting error messages. IMHO, such an approach is hopelessly fragile without error codes (or at least a well-defined set of possible errors for a given query). Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Attachment
nconway@klamath.dyndns.org (Neil Conway) writes:
>> Otherwise I'm okay with this, if we don't mind the probability of
>> breaking existing client applications that are looking for ExecAppend:
>> messages.
> I would be skeptical of any client application that tries to
> divine information about the result of an operation by inspecting
> error messages. IMHO, such an approach is hopelessly fragile
> without error codes (or at least a well-defined set of
> possible errors for a given query).
I agree with you that it's fragile --- but so far we have offered
clients no alternative, and it's not looking like we are going to have
an alternative in the near future. Like it or not, there are clients
out there that are looking at error strings, because they have no other
choice. I'm uncomfortable with the thought of breaking them for what's
really just a cosmetic code cleanup.
The ExecAppend family of messages are particularly nasty in this regard
because they include constraint-violation messages that are likely to
come up in normal operation (ie, they're data problems not query problems).
So they are likely candidates for clients to be checking for.
regards, tom lane
Patch applied. Thanks. --------------------------------------------------------------------------- Neil Conway wrote: > The attached patch fixes some spelling mistakes, makes the > comments on one of the optimizer functions a lot more > clear, adds a summary of the recent KSQO discussion to the > comments in the code, adds regression tests for the bug with > sequence state Tom fixed recently and another reg. test, and > removes some PostQuel legacy stuff: ExecAppend -> ExecInsert, > ExecRetrieve -> ExecSelect, etc. This was changed because the > elog() messages from this routine are user-visible, so we > should be using the SQL terms. > > Cheers, > > Neil > > -- > Neil Conway <neilconway@rogers.com> > PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Patch applied. Thanks.
Did you pay no attention to the subsequent discussion?
While I don't object to renaming the routines internally, I do have
strong doubts about changing the externally-visible error messages.
I'd suggest undoing the particular changes that pass routine names
to ExecConstraints, so that the error messages stay the same. We
can clean it up at some time *after* we offer error codes that clients
can test.
regards, tom lane
Bruce Momjian wrote: > > Patch applied. Thanks. Patch backed out. I had old version and needs cleanup. > > --------------------------------------------------------------------------- > > > > Neil Conway wrote: > > The attached patch fixes some spelling mistakes, makes the > > comments on one of the optimizer functions a lot more > > clear, adds a summary of the recent KSQO discussion to the > > comments in the code, adds regression tests for the bug with > > sequence state Tom fixed recently and another reg. test, and > > removes some PostQuel legacy stuff: ExecAppend -> ExecInsert, > > ExecRetrieve -> ExecSelect, etc. This was changed because the > > elog() messages from this routine are user-visible, so we > > should be using the SQL terms. > > > > Cheers, > > > > Neil > > > > -- > > Neil Conway <neilconway@rogers.com> > > PGP Key ID: DB3C29FC > > [ Attachment, skipping... ] > > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 6: Have you searched our list archives? > > > > http://archives.postgresql.org > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 853-3000 > + If your life is a hard drive, | 830 Blythe Avenue > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 > > > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Don't 'kill -9' the postmaster > > > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Patch applied. Thanks.
>
> Did you pay no attention to the subsequent discussion?
I thought that was related to a different patch but now I see it was the
same.
> While I don't object to renaming the routines internally, I do have
> strong doubts about changing the externally-visible error messages.
> I'd suggest undoing the particular changes that pass routine names
> to ExecConstraints, so that the error messages stay the same. We
> can clean it up at some time *after* we offer error codes that clients
> can test.
Well, with no error codes on the horizon, and schemas appearing to break
lots of stuff, I don't see the need to keep error messages consistent.
Heck, the error messages says:
elog(WARNING, "ExecReplace: replace can't run without transaction");
and we haven't had replace since 1994 or so. I think the cleanup is
needed.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> While I don't object to renaming the routines internally, I do have
>> strong doubts about changing the externally-visible error messages.
>> I'd suggest undoing the particular changes that pass routine names
>> to ExecConstraints, so that the error messages stay the same. We
>> can clean it up at some time *after* we offer error codes that clients
>> can test.
> Well, with no error codes on the horizon, and schemas appearing to break
> lots of stuff, I don't see the need to keep error messages consistent.
Schemas are NOT breaking clients that check to see which data-related
condition caused an insert/update to fail. I think it's important to
recognize the distinction between query-related errors (eg you
misspelled a column name) and data-related errors (the supplied value
failed a constraint check), because I believe client logic is much more
likely to contain pre-coded recovery behavior for specific types of
data errors.
Also, in places where we have changed error messages because of schemas,
there is a good feature-related reason to do so. This patch is adding
zero benefit as far as users are concerned, so I think its cost/benefit
ratio is too high to justify to users.
> Heck, the error messages says:
> elog(WARNING, "ExecReplace: replace can't run without transaction");
> and we haven't had replace since 1994 or so.
Yeah, and why do you think it hasn't been changed? Exactly this
consideration. I would probably have fixed those messages long ago
if I hadn't been worried about breaking clients.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> While I don't object to renaming the routines internally, I do have > >> strong doubts about changing the externally-visible error messages. > >> I'd suggest undoing the particular changes that pass routine names > >> to ExecConstraints, so that the error messages stay the same. We > >> can clean it up at some time *after* we offer error codes that clients > >> can test. > > > Well, with no error codes on the horizon, and schemas appearing to break > > lots of stuff, I don't see the need to keep error messages consistent. > > Schemas are NOT breaking clients that check to see which data-related > condition caused an insert/update to fail. I think it's important to > recognize the distinction between query-related errors (eg you > misspelled a column name) and data-related errors (the supplied value > failed a constraint check), because I believe client logic is much more > likely to contain pre-coded recovery behavior for specific types of > data errors. > > Also, in places where we have changed error messages because of schemas, > there is a good feature-related reason to do so. This patch is adding > zero benefit as far as users are concerned, so I think its cost/benefit > ratio is too high to justify to users. > > > Heck, the error messages says: > > elog(WARNING, "ExecReplace: replace can't run without transaction"); > > and we haven't had replace since 1994 or so. > > Yeah, and why do you think it hasn't been changed? Exactly this > consideration. I would probably have fixed those messages long ago > if I hadn't been worried about breaking clients. OK, does anyone know of any client that checks error message text in this area? There is that whole "prefix error message with function name" problem that maybe we should address at this point and fix them all. I know Peter has mentioned this but I don't remember the context. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> OK, does anyone know of any client that checks error message text in
> this area?
If you want to take a meaningful survey, you'd better ask the question
somewhere other than pgsql-patches. The folks who might have coded
such things are not likely reading this list.
regards, tom lane
OK, I have applied this patch, except for the changes that effect the elog messages. I will have to take a poll on general before I can make those changes. Thanks. --------------------------------------------------------------------------- Neil Conway wrote: > On Mon, Jun 24, 2002 at 04:53:17PM -0400, Tom Lane wrote: > > Neil Conway <nconway@klamath.dyndns.org> writes: > > *************** > > *** 251,257 **** > > ExecCheckRTPerms(parseTree->rtable, operation); > > > > /* > > ! * Search for subplans and APPEND nodes to check their rangetables. > > */ > > ExecCheckPlanPerms(plan, parseTree->rtable, operation); > > } > > --- 251,257 ---- > > ExecCheckRTPerms(parseTree->rtable, operation); > > > > /* > > ! * Search for subplans and INSERT nodes to check their rangetables. > > */ > > ExecCheckPlanPerms(plan, parseTree->rtable, operation); > > } > > *************** > > > > This comment was right beforehand and is so no longer :-(. > > Woops, s/append/insert/g was a bit over-enthusiastic. A revised > patch is attached (it's also updated for CVS HEAD). > > > Otherwise I'm okay with this, if we don't mind the probability of > > breaking existing client applications that are looking for ExecAppend: > > messages. > > I would be skeptical of any client application that tries to > divine information about the result of an operation by inspecting > error messages. IMHO, such an approach is hopelessly fragile > without error codes (or at least a well-defined set of > possible errors for a given query). -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Index: src/backend/executor/execMain.c =================================================================== RCS file: /var/lib/cvs/pgsql/src/backend/executor/execMain.c,v retrieving revision 1.165 diff -c -r1.165 execMain.c *** src/backend/executor/execMain.c 20 Jun 2002 20:29:27 -0000 1.165 --- src/backend/executor/execMain.c 25 Jun 2002 14:09:22 -0000 *************** *** 62,75 **** long numberTuples, ScanDirection direction, DestReceiver *destfunc); ! static void ExecRetrieve(TupleTableSlot *slot, DestReceiver *destfunc, EState *estate); ! static void ExecAppend(TupleTableSlot *slot, ItemPointer tupleid, EState *estate); static void ExecDelete(TupleTableSlot *slot, ItemPointer tupleid, EState *estate); ! static void ExecReplace(TupleTableSlot *slot, ItemPointer tupleid, EState *estate); static TupleTableSlot *EvalPlanQualNext(EState *estate); static void EndEvalPlanQual(EState *estate); --- 62,75 ---- long numberTuples, ScanDirection direction, DestReceiver *destfunc); ! static void ExecSelect(TupleTableSlot *slot, DestReceiver *destfunc, EState *estate); ! static void ExecInsert(TupleTableSlot *slot, ItemPointer tupleid, EState *estate); static void ExecDelete(TupleTableSlot *slot, ItemPointer tupleid, EState *estate); ! static void ExecUpdate(TupleTableSlot *slot, ItemPointer tupleid, EState *estate); static TupleTableSlot *EvalPlanQualNext(EState *estate); static void EndEvalPlanQual(EState *estate); *************** *** 583,589 **** /* * Get the tuple descriptor describing the type of tuples to return. * (this is especially important if we are creating a relation with ! * "retrieve into") */ tupType = ExecGetTupType(plan); /* tuple descriptor */ --- 583,589 ---- /* * Get the tuple descriptor describing the type of tuples to return. * (this is especially important if we are creating a relation with ! * "SELECT INTO") */ tupType = ExecGetTupType(plan); /* tuple descriptor */ *************** *** 892,898 **** * Retrieves all tuples if numberTuples is 0 * * result is either a slot containing the last tuple in the case ! * of a RETRIEVE or NULL otherwise. * * Note: the ctid attribute is a 'junk' attribute that is removed before the * user can see it --- 892,898 ---- * Retrieves all tuples if numberTuples is 0 * * result is either a slot containing the last tuple in the case ! * of a SELECT or NULL otherwise. * * Note: the ctid attribute is a 'junk' attribute that is removed before the * user can see it *************** *** 1068,1096 **** slot = ExecStoreTuple(newTuple, /* tuple to store */ junkfilter->jf_resultSlot, /* dest slot */ ! InvalidBuffer, /* this tuple has no ! * buffer */ true); /* tuple should be pfreed */ ! } /* if (junkfilter... */ /* * now that we have a tuple, do the appropriate thing with it.. * either return it to the user, add it to a relation someplace, * delete it from a relation, or modify some of its attributes. */ - switch (operation) { case CMD_SELECT: ! ExecRetrieve(slot, /* slot containing tuple */ ! destfunc, /* destination's tuple-receiver ! * obj */ ! estate); /* */ result = slot; break; case CMD_INSERT: ! ExecAppend(slot, tupleid, estate); result = NULL; break; --- 1068,1093 ---- slot = ExecStoreTuple(newTuple, /* tuple to store */ junkfilter->jf_resultSlot, /* dest slot */ ! InvalidBuffer, /* this tuple has no buffer */ true); /* tuple should be pfreed */ ! } /* * now that we have a tuple, do the appropriate thing with it.. * either return it to the user, add it to a relation someplace, * delete it from a relation, or modify some of its attributes. */ switch (operation) { case CMD_SELECT: ! ExecSelect(slot, /* slot containing tuple */ ! destfunc, /* destination's tuple-receiver obj */ ! estate); result = slot; break; case CMD_INSERT: ! ExecInsert(slot, tupleid, estate); result = NULL; break; *************** *** 1100,1106 **** break; case CMD_UPDATE: ! ExecReplace(slot, tupleid, estate); result = NULL; break; --- 1097,1103 ---- break; case CMD_UPDATE: ! ExecUpdate(slot, tupleid, estate); result = NULL; break; *************** *** 1121,1145 **** /* * here, result is either a slot containing a tuple in the case of a ! * RETRIEVE or NULL otherwise. */ return result; } /* ---------------------------------------------------------------- ! * ExecRetrieve * ! * RETRIEVEs are easy.. we just pass the tuple to the appropriate * print function. The only complexity is when we do a ! * "retrieve into", in which case we insert the tuple into * the appropriate relation (note: this is a newly created relation * so we don't need to worry about indices or locks.) * ---------------------------------------------------------------- */ static void ! ExecRetrieve(TupleTableSlot *slot, ! DestReceiver *destfunc, ! EState *estate) { HeapTuple tuple; TupleDesc attrtype; --- 1118,1142 ---- /* * here, result is either a slot containing a tuple in the case of a ! * SELECT or NULL otherwise. */ return result; } /* ---------------------------------------------------------------- ! * ExecSelect * ! * SELECTs are easy.. we just pass the tuple to the appropriate * print function. The only complexity is when we do a ! * "SELECT INTO", in which case we insert the tuple into * the appropriate relation (note: this is a newly created relation * so we don't need to worry about indices or locks.) * ---------------------------------------------------------------- */ static void ! ExecSelect(TupleTableSlot *slot, ! DestReceiver *destfunc, ! EState *estate) { HeapTuple tuple; TupleDesc attrtype; *************** *** 1169,1184 **** } /* ---------------------------------------------------------------- ! * ExecAppend * ! * APPENDs are trickier.. we have to insert the tuple into * the base relation and insert appropriate tuples into the * index relations. * ---------------------------------------------------------------- */ - static void ! ExecAppend(TupleTableSlot *slot, ItemPointer tupleid, EState *estate) { --- 1166,1180 ---- } /* ---------------------------------------------------------------- ! * ExecInsert * ! * INSERTs are trickier.. we have to insert the tuple into * the base relation and insert appropriate tuples into the * index relations. * ---------------------------------------------------------------- */ static void ! ExecInsert(TupleTableSlot *slot, ItemPointer tupleid, EState *estate) { *************** *** 1227,1233 **** * Check the constraints of the tuple */ if (resultRelationDesc->rd_att->constr) ! ExecConstraints("ExecAppend", resultRelInfo, slot, estate); /* * insert the tuple --- 1223,1229 ---- * Check the constraints of the tuple */ if (resultRelationDesc->rd_att->constr) ! ExecConstraints("ExecInsert", resultRelInfo, slot, estate); /* * insert the tuple *************** *** 1259,1265 **** /* ---------------------------------------------------------------- * ExecDelete * ! * DELETE is like append, we delete the tuple and its * index tuples. * ---------------------------------------------------------------- */ --- 1255,1261 ---- /* ---------------------------------------------------------------- * ExecDelete * ! * DELETE is like UPDATE, we delete the tuple and its * index tuples. * ---------------------------------------------------------------- */ *************** *** 1346,1363 **** } /* ---------------------------------------------------------------- ! * ExecReplace * ! * note: we can't run replace queries with transactions ! * off because replaces are actually appends and our ! * scan will mistakenly loop forever, replacing the tuple ! * it just appended.. This should be fixed but until it * is, we don't want to get stuck in an infinite loop * which corrupts your database.. * ---------------------------------------------------------------- */ static void ! ExecReplace(TupleTableSlot *slot, ItemPointer tupleid, EState *estate) { --- 1342,1359 ---- } /* ---------------------------------------------------------------- ! * ExecUpdate * ! * note: we can't run UPDATE queries with transactions ! * off because UPDATEs are actually INSERTs and our ! * scan will mistakenly loop forever, updating the tuple ! * it just inserted.. This should be fixed but until it * is, we don't want to get stuck in an infinite loop * which corrupts your database.. * ---------------------------------------------------------------- */ static void ! ExecUpdate(TupleTableSlot *slot, ItemPointer tupleid, EState *estate) { *************** *** 1472,1478 **** /* * Note: instead of having to update the old index tuples associated * with the heap tuple, all we do is form and insert new index tuples. ! * This is because replaces are actually deletes and inserts and index * tuple deletion is done automagically by the vacuum daemon. All we * do is insert new index tuples. -cim 9/27/89 */ --- 1468,1474 ---- /* * Note: instead of having to update the old index tuples associated * with the heap tuple, all we do is form and insert new index tuples. ! * This is because UPDATEs are actually DELETEs and INSERTs and index * tuple deletion is done automagically by the vacuum daemon. All we * do is insert new index tuples. -cim 9/27/89 */ *************** *** 1481,1487 **** * process indices * * heap_update updates a tuple in the base relation by invalidating it ! * and then appending a new tuple to the relation. As a side effect, * the tupleid of the new tuple is placed in the new tuple's t_ctid * field. So we now insert index tuples using the new tupleid stored * there. --- 1477,1483 ---- * process indices * * heap_update updates a tuple in the base relation by invalidating it ! * and then inserting a new tuple to the relation. As a side effect, * the tupleid of the new tuple is placed in the new tuple's t_ctid * field. So we now insert index tuples using the new tupleid stored * there. *************** *** 1554,1560 **** } void ! ExecConstraints(char *caller, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) { Relation rel = resultRelInfo->ri_RelationDesc; --- 1550,1556 ---- } void ! ExecConstraints(const char *caller, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) { Relation rel = resultRelInfo->ri_RelationDesc; Index: src/backend/executor/execUtils.c =================================================================== RCS file: /var/lib/cvs/pgsql/src/backend/executor/execUtils.c,v retrieving revision 1.83 diff -c -r1.83 execUtils.c *** src/backend/executor/execUtils.c 20 Jun 2002 20:29:28 -0000 1.83 --- src/backend/executor/execUtils.c 25 Jun 2002 14:09:22 -0000 *************** *** 18,24 **** * * ExecOpenIndices \ * ExecCloseIndices | referenced by InitPlan, EndPlan, ! * ExecInsertIndexTuples / ExecAppend, ExecReplace * * RegisterExprContextCallback Register function shutdown callback * UnregisterExprContextCallback Deregister function shutdown callback --- 18,24 ---- * * ExecOpenIndices \ * ExecCloseIndices | referenced by InitPlan, EndPlan, ! * ExecInsertIndexTuples / ExecInsert, ExecUpdate * * RegisterExprContextCallback Register function shutdown callback * UnregisterExprContextCallback Deregister function shutdown callback Index: src/backend/optimizer/path/costsize.c =================================================================== RCS file: /var/lib/cvs/pgsql/src/backend/optimizer/path/costsize.c,v retrieving revision 1.85 diff -c -r1.85 costsize.c *** src/backend/optimizer/path/costsize.c 20 Jun 2002 20:29:29 -0000 1.85 --- src/backend/optimizer/path/costsize.c 25 Jun 2002 14:09:22 -0000 *************** *** 154,164 **** * * Given a guesstimated cache size, we estimate the actual I/O cost per page * with the entirely ad-hoc equations: ! * for rel_size <= effective_cache_size: ! * 1 + (random_page_cost/2-1) * (rel_size/effective_cache_size) ** 2 ! * for rel_size >= effective_cache_size: ! * random_page_cost * (1 - (effective_cache_size/rel_size)/2) ! * These give the right asymptotic behavior (=> 1.0 as rel_size becomes * small, => random_page_cost as it becomes large) and meet in the middle * with the estimate that the cache is about 50% effective for a relation * of the same size as effective_cache_size. (XXX this is probably all --- 154,164 ---- * * Given a guesstimated cache size, we estimate the actual I/O cost per page * with the entirely ad-hoc equations: ! * if relpages >= effective_cache_size: ! * random_page_cost * (1 - (effective_cache_size/relpages)/2) ! * if relpages < effective_cache_size: ! * 1 + (random_page_cost/2-1) * (relpages/effective_cache_size) ** 2 ! * These give the right asymptotic behavior (=> 1.0 as relpages becomes * small, => random_page_cost as it becomes large) and meet in the middle * with the estimate that the cache is about 50% effective for a relation * of the same size as effective_cache_size. (XXX this is probably all Index: src/backend/optimizer/prep/_deadcode/prepkeyset.c =================================================================== RCS file: /var/lib/cvs/pgsql/src/backend/optimizer/prep/_deadcode/prepkeyset.c,v retrieving revision 1.2 diff -c -r1.2 prepkeyset.c *** src/backend/optimizer/prep/_deadcode/prepkeyset.c 20 Jun 2002 20:29:31 -0000 1.2 --- src/backend/optimizer/prep/_deadcode/prepkeyset.c 25 Jun 2002 14:09:22 -0000 *************** *** 1,7 **** /*------------------------------------------------------------------------- * * prepkeyset.c ! * Special preperation for keyset queries. * * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California --- 1,7 ---- /*------------------------------------------------------------------------- * * prepkeyset.c ! * Special preparation for keyset queries (KSQO). * * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California *************** *** 14,25 **** #include "postgres.h" #include "optimizer/planmain.h" - /* - * Node_Copy - * a macro to simplify calling of copyObject on the specified field - */ - #define Node_Copy(from, newnode, field) newnode->field = copyObject(from->field) - bool _use_keyset_query_optimizer = FALSE; #ifdef ENABLE_KEY_SET_QUERY --- 14,19 ---- *************** *** 55,67 **** * a HAVING, or a GROUP BY. It must be a single table and have KSQO * set to 'on'. * ! * The primary use of this transformation is to avoid the exponrntial * memory consumption of cnfify() and to make use of index access * methods. * * daveh@insightdist.com 1998-08-31 * * May want to also prune out duplicate terms. **********************************************************************/ void transformKeySetQuery(Query *origNode) --- 49,68 ---- * a HAVING, or a GROUP BY. It must be a single table and have KSQO * set to 'on'. * ! * The primary use of this transformation is to avoid the exponential * memory consumption of cnfify() and to make use of index access * methods. * * daveh@insightdist.com 1998-08-31 * * May want to also prune out duplicate terms. + * + * XXX: this code is currently not compiled because it has not been + * updated to work with the re-implementation of UNION/INTERSECT/EXCEPT + * in PostgreSQL 7.1. However, it is of questionable value in any + * case, because it changes the semantics of the original query: + * UNION will add an implicit SELECT DISTINCT, which might change + * the results that are returned. **********************************************************************/ void transformKeySetQuery(Query *origNode) Index: src/include/executor/executor.h =================================================================== RCS file: /var/lib/cvs/pgsql/src/include/executor/executor.h,v retrieving revision 1.66 diff -c -r1.66 executor.h *** src/include/executor/executor.h 20 Jun 2002 20:29:49 -0000 1.66 --- src/include/executor/executor.h 25 Jun 2002 14:09:22 -0000 *************** *** 52,58 **** extern TupleTableSlot *ExecutorRun(QueryDesc *queryDesc, EState *estate, ScanDirection direction, long count); extern void ExecutorEnd(QueryDesc *queryDesc, EState *estate); ! extern void ExecConstraints(char *caller, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate); extern TupleTableSlot *EvalPlanQual(EState *estate, Index rti, ItemPointer tid); --- 52,58 ---- extern TupleTableSlot *ExecutorRun(QueryDesc *queryDesc, EState *estate, ScanDirection direction, long count); extern void ExecutorEnd(QueryDesc *queryDesc, EState *estate); ! extern void ExecConstraints(const char *caller, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate); extern TupleTableSlot *EvalPlanQual(EState *estate, Index rti, ItemPointer tid); Index: src/test/regress/expected/create_misc.out =================================================================== RCS file: /var/lib/cvs/pgsql/src/test/regress/expected/create_misc.out,v retrieving revision 1.13 diff -c -r1.13 create_misc.out *** src/test/regress/expected/create_misc.out 6 Mar 2002 06:10:52 -0000 1.13 --- src/test/regress/expected/create_misc.out 25 Jun 2002 14:09:23 -0000 *************** *** 151,153 **** --- 151,163 ---- force | 100 (3 rows) + CREATE SEQUENCE sequence_test; + BEGIN; + SELECT nextval('sequence_test'); + nextval + --------- + 1 + (1 row) + + DROP SEQUENCE sequence_test; + END; Index: src/test/regress/expected/select_having.out =================================================================== RCS file: /var/lib/cvs/pgsql/src/test/regress/expected/select_having.out,v retrieving revision 1.4 diff -c -r1.4 select_having.out *** src/test/regress/expected/select_having.out 6 Jan 2000 06:40:54 -0000 1.4 --- src/test/regress/expected/select_having.out 25 Jun 2002 14:09:23 -0000 *************** *** 21,26 **** --- 21,35 ---- 3 | bbbb (2 rows) + -- HAVING is equivalent to WHERE in this case + SELECT b, c FROM test_having + GROUP BY b, c HAVING b = 3; + b | c + ---+---------- + 3 | BBBB + 3 | bbbb + (2 rows) + SELECT lower(c), count(c) FROM test_having GROUP BY lower(c) HAVING count(*) > 2 OR min(a) = max(a); lower | count Index: src/test/regress/sql/create_misc.sql =================================================================== RCS file: /var/lib/cvs/pgsql/src/test/regress/sql/create_misc.sql,v retrieving revision 1.9 diff -c -r1.9 create_misc.sql *** src/test/regress/sql/create_misc.sql 21 May 2001 16:54:46 -0000 1.9 --- src/test/regress/sql/create_misc.sql 25 Jun 2002 14:09:23 -0000 *************** *** 217,219 **** --- 217,226 ---- INSERT INTO serialTest VALUES ('wrong', NULL); SELECT * FROM serialTest; + + CREATE SEQUENCE sequence_test; + + BEGIN; + SELECT nextval('sequence_test'); + DROP SEQUENCE sequence_test; + END; Index: src/test/regress/sql/select_having.sql =================================================================== RCS file: /var/lib/cvs/pgsql/src/test/regress/sql/select_having.sql,v retrieving revision 1.4 diff -c -r1.4 select_having.sql *** src/test/regress/sql/select_having.sql 6 Jan 2000 06:41:55 -0000 1.4 --- src/test/regress/sql/select_having.sql 25 Jun 2002 14:09:23 -0000 *************** *** 18,23 **** --- 18,27 ---- SELECT b, c FROM test_having GROUP BY b, c HAVING count(*) = 1; + -- HAVING is equivalent to WHERE in this case + SELECT b, c FROM test_having + GROUP BY b, c HAVING b = 3; + SELECT lower(c), count(c) FROM test_having GROUP BY lower(c) HAVING count(*) > 2 OR min(a) = max(a);
After no adverse comments from the 'general' list, I have applied this part of the patch. Should we consider getting rid of the Exec part, so it just says "Insert/Update" rather than "ExecInsert/ExecUpdate"? I assume the INSERT/UPDATE is needed because we are reporting trigger activity. --------------------------------------------------------------------------- Tom Lane wrote: > Neil Conway <nconway@klamath.dyndns.org> writes: > > *************** > *** 251,257 **** > ExecCheckRTPerms(parseTree->rtable, operation); > > /* > ! * Search for subplans and APPEND nodes to check their rangetables. > */ > ExecCheckPlanPerms(plan, parseTree->rtable, operation); > } > --- 251,257 ---- > ExecCheckRTPerms(parseTree->rtable, operation); > > /* > ! * Search for subplans and INSERT nodes to check their rangetables. > */ > ExecCheckPlanPerms(plan, parseTree->rtable, operation); > } > *************** > > This comment was right beforehand and is so no longer :-(. > > Otherwise I'm okay with this, if we don't mind the probability of > breaking existing client applications that are looking for ExecAppend: > messages. One might think that unnecessary changes in common error > messages are not a good idea until sometime after we've implemented an > error code facility and given people a chance to move over to looking > at error codes instead of error strings. > > regards, tom lane > > > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > > > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
On Thu, Jul 11, 2002 at 05:32:13PM -0400, Bruce Momjian wrote: > After no adverse comments from the 'general' list, I have applied this > part of the patch. Should we consider getting rid of the Exec part, so > it just says "Insert/Update" rather than "ExecInsert/ExecUpdate"? Well, I think the practice of reporting C function names for user-visible error messages isn't a particularly good idea -- but if we'd like to fix that, this is only one of numerous error messages that need to be corrected. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Neil Conway wrote: > On Thu, Jul 11, 2002 at 05:32:13PM -0400, Bruce Momjian wrote: > > After no adverse comments from the 'general' list, I have applied this > > part of the patch. Should we consider getting rid of the Exec part, so > > it just says "Insert/Update" rather than "ExecInsert/ExecUpdate"? > > Well, I think the practice of reporting C function names for > user-visible error messages isn't a particularly good idea -- but if > we'd like to fix that, this is only one of numerous error messages that > need to be corrected. > Is there any desire for this to be done? I am willing to remove all the function name references in the code and replace them with meaningful text if needed. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> Well, I think the practice of reporting C function names for
>> user-visible error messages isn't a particularly good idea -- but if
>> we'd like to fix that, this is only one of numerous error messages that
>> need to be corrected.
> Is there any desire for this to be done?
I am *strongly* against removing those names until such time as we have
a substitute mechanism for identifying where error messages come from
in the source. We've talked before about making __FILE__/__LINE__ info
available as a secondary field in error messages, as part of a general
overhaul of error reporting ... but no one seems to want to tackle it...
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Well, I think the practice of reporting C function names for > >> user-visible error messages isn't a particularly good idea -- but if > >> we'd like to fix that, this is only one of numerous error messages that > >> need to be corrected. > > > Is there any desire for this to be done? > > I am *strongly* against removing those names until such time as we have > a substitute mechanism for identifying where error messages come from > in the source. We've talked before about making __FILE__/__LINE__ info > available as a secondary field in error messages, as part of a general > overhaul of error reporting ... but no one seems to want to tackle it... Seems we could do it with gcc and ANSI compilers pretty easily by making elog a macro and prepending __FILE__ etc in there somewhere. Did Peter research that. Do we actually use the function names in a meaningful way just for error messages that could come from multiple places, or it is petty much a hodge-podge? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Do we actually use the function names in a meaningful way just for error
> messages that could come from multiple places, or it is petty much a
> hodge-podge?
I don't deny that it's a hodge-podge ;-). But we do have a huge number
of fairly similar messages, for example "foo: cache lookup failed for ..."
and the presence of the function name is a big leg up in diagnosing
stuff remotely. (If you can make it happen in a debugging situation,
gdb can provide the info, but that's a luxury we don't always have.)
I am sure there are some cases where the function name could be removed
today without loss of info, because the message is unique anyway. I was
objecting to the implication that you were going to engage in a massive
removal of function names without concern for loss of debuggability...
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Do we actually use the function names in a meaningful way just for error > > messages that could come from multiple places, or it is petty much a > > hodge-podge? > > I don't deny that it's a hodge-podge ;-). But we do have a huge number > of fairly similar messages, for example "foo: cache lookup failed for ..." > and the presence of the function name is a big leg up in diagnosing > stuff remotely. (If you can make it happen in a debugging situation, > gdb can provide the info, but that's a luxury we don't always have.) > > I am sure there are some cases where the function name could be removed > today without loss of info, because the message is unique anyway. I was > objecting to the implication that you were going to engage in a massive > removal of function names without concern for loss of debuggability... Yea, if it provides value in some situations, it is not worth touching them, _and_ requiring all the translators to hit them too. When we can fit it completely, I will do it. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian writes: > OK, does anyone know of any client that checks error message text in > this area? There is that whole "prefix error message with function > name" problem that maybe we should address at this point and fix them > all. I know Peter has mentioned this but I don't remember the context. I think we should attack error codes before doing a wholesale cleanup of error message wording in the server. (And when we do the latter, we should define a style guide before doing "several minor cleanups".) -- Peter Eisentraut peter_e@gmx.net
Peter Eisentraut <peter_e@gmx.net> writes:
> I think we should attack error codes before doing a wholesale cleanup of
> error message wording in the server.
Error codes, __FILE__/__LINE__ info, and syntax error positions are all
one big item in my mind; we should do all that stuff together, and then
there will be a lot of wording revisions we can undertake.
> we should define a style guide before doing "several minor cleanups".
Yup.
regards, tom lane