pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Artur Zakirov <a.zakirov@postgrespro.ru>
Cc: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: Alex Ignatov <a.ignatov@postgrespro.ru>
Cc: Bruce Momjian <bruce@momjian.us>
Cc: amul sul <sul_amul@yahoo.co.in>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Bug in to_timestamp().
Date: Wed, 28 Sep 2016 17:18:51 -0400
Message-ID: <29142.1475097531@sss.pgh.pa.us> (raw)
In-Reply-To: <b2a39359-3282-b402-f4a3-057aae500ee7@postgrespro.ru>
References: <1873520224.1784572.1465833145330.JavaMail.yahoo.ref@mail.yahoo.com>
	<1873520224.1784572.1465833145330.JavaMail.yahoo@mail.yahoo.com>
	<f64563b3-5c3c-6f28-3f73-41b2159417bc@postgrespro.ru>
	<884791796.213185.1466667686999.JavaMail.yahoo@mail.yahoo.com>
	<20160623133001.GF21246@momjian.us>
	<1877e60a-a970-18b5-453c-e342f96150e6@postgrespro.ru>
	<CAKFQuwZKtS0Ad=a1E9_mkdYwLMq+JA3YjmhCvjJ1JF+3BU8a0g@mail.gmail.com>
	<d469b177-fd4d-5a5b-769e-620957ec1a64@postgrespro.ru>
	<CAKFQuwb2gp8n2ai_agyJod5BTYxL7ek0AjB9zifDSceK6QntnQ@mail.gmail.com>
	<CA+TgmoaWTgbtd2RTwVvySMVoOcHmWq66rCCxtVywOOX3FJy4tQ@mail.gmail.com>
	<31915.1466703655@sss.pgh.pa.us>
	<CA+TgmoaFQZeqmAo3CdivNu70coB=9eM5OroKYtNcTuMaC1kuUQ@mail.gmail.com>
	<32684.1466704957@sss.pgh.pa.us>
	<734e50cd-c563-4afd-d1c8-80f244b3265e@postgrespro.ru>
	<CAFj8pRDGxe=cdcZ7pur=2WeW9O3MQCZTBEAOJfQ7OLQJaYXDvw@mail.gmail.com>
	<b2a39359-3282-b402-f4a3-057aae500ee7@postgrespro.ru>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>

Artur Zakirov <a.zakirov@postgrespro.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.

			regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



view thread (138+ messages)  latest in thread

Message-ID: <29142.1475097531@sss.pgh.pa.us>
Permalink:  ../29142.1475097531@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/29142.1475097531@sss.pgh.pa.us

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, a.zakirov@postgrespro.ru, pavel.stehule@gmail.com, robertmhaas@gmail.com, david.g.johnston@gmail.com, a.ignatov@postgrespro.ru, bruce@momjian.us, sul_amul@yahoo.co.in
  Subject: Re: Bug in to_timestamp().
  In-Reply-To: <29142.1475097531@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox