Re: Bug in to_timestamp().

On Thu, Sep 29, 2016 at 4:54 AM, amul sul <sulamul(at)gmail(dot)com> wrote:
> On Thu, Sep 29, 2016 at 2:48 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> writes:
>>> - now DCH_cache_getnew() is called after parse_format(). Because now
>>> parse_format() can raise an error and in the next attempt
>>> DCH_cache_search() could return broken cache entry.
>>
>> 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).
>>
>> 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?

You may be able to set the status back to "Ready for Committer", or
else you can move the entry to the next CommitFest and do it there.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company