From: | Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> |
---|---|
To: | amul sul <sulamul(at)gmail(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Alex Ignatov <a(dot)ignatov(at)postgrespro(dot)ru>, Bruce Momjian <bruce(at)momjian(dot)us>, amul sul <sul_amul(at)yahoo(dot)co(dot)in>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Bug in to_timestamp(). |
Date: | 2016-09-29 09:53:34 |
Message-ID: | CAKNkYny-0YJZzz=a9E==UBuiEoMAFV=n6tik7oLbDVtahiW7tA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2016-09-29 13:54 GMT+05:00 amul sul <sulamul(at)gmail(dot)com>:
>
> On Thu, Sep 29, 2016 at 2:48 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > I started looking at your 0001-to-timestamp-format-checking-v4.patch
> > and this point immediately jumped out at me. Currently the code relies
> > ... without any documentation ... on no elog being thrown out of
> > parse_format(). That's at the very least trouble waiting to happen.
> > There's a hack to deal with errors from within the NUMDesc_prepare
> > subroutine, but it's a pretty ugly and underdocumented hack. And what
> > you had here was randomly different from that solution, too.
> >
> > After a bit of thought it seemed to me that a much cleaner fix is to add
> > a "valid" flag to the cache entries, which we can leave clear until we
> > have finished parsing the new format string. That avoids adding extra
> > data copying as you suggested, removes the need for PG_TRY, and just
> > generally seems cleaner and more bulletproof.
> >
> > I've pushed a patch that does it that way. The 0001 patch will need
> > to be rebased over that (might just require removal of some hunks,
> > not sure).
> >
> > I also pushed 0002-to-timestamp-validation-v2.patch with some revisions
> > (it'd broken acceptance of BC dates, among other things, but I think
> > I fixed everything).
Thank you for committing the 0002 part of the patch! I wanted to fix
cache functions too, but wasn't sure about that.
> >
> > Since you told us earlier that you'd be on vacation through the end of
> > September, I'm assuming that nothing more will happen on this patch during
> > this commitfest, so I will mark the CF entry Returned With Feedback.
>
> Behalf of Artur I've rebased patch, removed hunk dealing with broken
> cache entries by copying it, which is no longer required after 83bed06
> commit.
>
> Commitfest status left untouched, I am not sure how to deal with
> "Returned With Feedback" patch. Is there any chance that, this can be
> considered again for committer review?
Thank you for fixing the patch!
Today I have access to the internet and able to fix and test the
patch. I've looked at your 0001-to-timestamp-format-checking-v5.patch.
It seems nice to me. I suppose it is necessary to fix
is_char_separator() declaration.
from:
static bool is_char_separator(char *str);
to:
static bool is_char_separator(const char *str);
Because now in parse_format() *str argument is const.
I attached new version of the patch, which fix is_char_separator()
declaration too.
Sorry for confusing!
--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Attachment | Content-Type | Size |
---|---|---|
0001-to-timestamp-format-checking-v6.patch | application/octet-stream | 15.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2016-09-29 10:17:53 | Re: less expensive pg_buffercache on big shmem |
Previous Message | Heikki Linnakangas | 2016-09-29 09:49:27 | Re: Tuplesort merge pre-reading |