quoting psql varible as identifier
in [PgSql]
|
Prev: Port for Microsoft Services for Unix (SFU) or SUA
Next: [HACKERS] solution to make static changes in pg_hba.conf file?
From: Pavel Stehule on 22 Jan 2010 02:16 2010/1/22 Robert Haas <robertmhaas(a)gmail.com>: > On Thu, Jan 21, 2010 at 2:25 PM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >> 2010/1/21 Robert Haas <robertmhaas(a)gmail.com>: >>> On Thu, Jan 21, 2010 at 12:53 PM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >>>> add to state structure field like lexer_error. This field will be >>>> checked before execution >>>> it could be ugly for metacommands, there will be lot of new checks :( >>> >>> Eh? Â The only places where we should need new tests are the places >>> that check PQExpBufferBroken() now - there are only 6 calls to that >>> function in src/bin/psql and not all of them need to be changed. Â The >>> places that do need to be changed will need to be modified to check >>> PQExpBufferBroken() || lexer_coughed_up_a_lung. >> >> no, it is only 6 calls because we don't check psql_scan_slash_option result. > > psql_scan_slash_option() already has a way to signal errors - it can > return NULL. Â Type any backslash command followed by a single quote.... NULL means "no value". But I thing, so there is only one important - \set. For other can be enough some error message and empty value. > > I'm not saying I love the way those errors are handled, but if we make > this patch about revising the way psql does error handling, this is > not going to get committed for this release... Â what we need to do is > fit what we're trying to do into the existing model. > I try to find some simple and I'll send a patch. Pavel > ...Robert > -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 22 Jan 2010 07:19 Hello here is new variant. Add scan_state flag "valid" and enhance protection against execution broken statement. Regards Pavel Stehule 2010/1/22 Pavel Stehule <pavel.stehule(a)gmail.com>: > 2010/1/22 Robert Haas <robertmhaas(a)gmail.com>: >> On Thu, Jan 21, 2010 at 2:25 PM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >>> 2010/1/21 Robert Haas <robertmhaas(a)gmail.com>: >>>> On Thu, Jan 21, 2010 at 12:53 PM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >>>>> add to state structure field like lexer_error. This field will be >>>>> checked before execution >>>>> it could be ugly for metacommands, there will be lot of new checks :( >>>> >>>> Eh? Â The only places where we should need new tests are the places >>>> that check PQExpBufferBroken() now - there are only 6 calls to that >>>> function in src/bin/psql and not all of them need to be changed. Â The >>>> places that do need to be changed will need to be modified to check >>>> PQExpBufferBroken() || lexer_coughed_up_a_lung. >>> >>> no, it is only 6 calls because we don't check psql_scan_slash_option result. >> >> psql_scan_slash_option() already has a way to signal errors - it can >> return NULL. Â Type any backslash command followed by a single quote.... > > NULL means "no value". But I thing, so there is only one important - > \set. For other can be enough some error message and empty value. > >> >> I'm not saying I love the way those errors are handled, but if we make >> this patch about revising the way psql does error handling, this is >> not going to get committed for this release... Â what we need to do is >> fit what we're trying to do into the existing model. >> > > I try to find some simple and I'll send a patch. > > Pavel > >> ...Robert >> >
From: Robert Haas on 22 Jan 2010 14:20 On Fri, Jan 22, 2010 at 7:19 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: > here is new variant. Add scan_state flag "valid" and enhance > protection against execution broken statement. This doesn't make sense to me. You've changed the way \set is handled - in a way that doesn't seem particularly appropriate to me - but most of the other backslash commands are unmodified - but then there's this hack at the bottom that overrides the return value if psql_scan_is_valid() fails. And then there's this: - /* we need not do psql_scan_reset() here */ + psql_scan_reset(scan_state); It's extremely unclear what the rationale for this change is. Basically, you need to either improve the comments in here so that someone can understand what is going on, or you need to submit it with some detailed documentation explaining the rationale behind each change and why it is right, or more than likely both. But I think the whole approach is likely off-base and you need to go back and think about a cleaner way to get this done. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 24 Jan 2010 03:57 2010/1/22 Robert Haas <robertmhaas(a)gmail.com>: > On Fri, Jan 22, 2010 at 7:19 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >> here is new variant. Add scan_state flag "valid" and enhance >> protection against execution broken statement. > > This doesn't make sense to me. Â You've changed the way \set is handled > - in a way that doesn't seem particularly appropriate to me - but most > of the other backslash commands are unmodified - but then there's this > hack at the bottom that overrides the return value if > psql_scan_is_valid() fails. Â And then there's this: > > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* we need not do psql_scan_reset() here */ > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â psql_scan_reset(scan_state); > > It's extremely unclear what the rationale for this change is. Sure, I'll enhance comments. All \command is verified on broken scanning. So when command's processing time the scanning is broken, then we can detect it on the end - flag valid is changed only to false direction. \set is different - it only one statement, that store values. And it is reason why I did more checks there. I am not sure - it is necessary now - simply we can set ERROR on the end and finish. psql_scan_reset is called on the end of any statement - so we can there reset info about scanning. I'll look on it tomorrow. Regards Pavel Stehule > > Basically, you need to either improve the comments in here so that > someone can understand what is going on, or you need to submit it with > some detailed documentation explaining the rationale behind each > change and why it is right, or more than likely both. Â But I think the > whole approach is likely off-base and you need to go back and think > about a cleaner way to get this done. > > ...Robert > -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 25 Jan 2010 07:36
Hello I hope, so this version is more readable and more clean. I removed some not necessary checks. regards Pavel 2010/1/22 Robert Haas <robertmhaas(a)gmail.com>: > On Fri, Jan 22, 2010 at 7:19 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >> here is new variant. Add scan_state flag "valid" and enhance >> protection against execution broken statement. > > This doesn't make sense to me. Â You've changed the way \set is handled > - in a way that doesn't seem particularly appropriate to me - but most > of the other backslash commands are unmodified - but then there's this > hack at the bottom that overrides the return value if > psql_scan_is_valid() fails. Â And then there's this: > > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* we need not do psql_scan_reset() here */ > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â psql_scan_reset(scan_state); > > It's extremely unclear what the rationale for this change is. > > Basically, you need to either improve the comments in here so that > someone can understand what is going on, or you need to submit it with > some detailed documentation explaining the rationale behind each > change and why it is right, or more than likely both. Â But I think the > whole approach is likely off-base and you need to go back and think > about a cleaner way to get this done. > > ...Robert > |