several minor cleanups

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

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

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