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

help / color / mirror / Atom feed
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Cc: Arthur Zakirov <zaartur@gmail.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Allow to_date() and to_timestamp() to accept localized names
Date: Tue, 21 Jan 2020 18:20:24 -0500
Message-ID: <31691.1579648824@sss.pgh.pa.us> (raw)
In-Reply-To: <CAC+AXB2a6ei0mPFG7h-NaiBVO5=tKT67rONZmrDzoOcAE-B2Sg@mail.gmail.com>
References: <CAC+AXB2JOe4Dw1teZP-N-isNyDdY3F96oGFVYFxhmWtsbgi=ig@mail.gmail.com>
	<20190925195745.GA21894@alvherre.pgsql>
	<CAC+AXB12A7Fr9djoq7f1-WYesnZTRKpGpHM12MRG+M00mewN9g@mail.gmail.com>
	<20200111160602.4wwvyl7lo3oxoewf@development>
	<CAC+AXB1rLC7Gu7-fTQPNwSY9sFC9hkpn8_J3WyGwaKXvNZnoVQ@mail.gmail.com>
	<ba6fa1fd-ad60-d1ea-7226-676790c108ca@gmail.com>
	<CAC+AXB2a6ei0mPFG7h-NaiBVO5=tKT67rONZmrDzoOcAE-B2Sg@mail.gmail.com>

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> [ 0001-Allow-localized-month-names-to_date-v6.patch ]

I took a quick look through this.

One thing I completely don't understand is why it's sufficient for
seq_search_localized to do "initcap" semantics.  Shouldn't it cover
the all-upper and all-lower cases as well, as the existing seq_search
code does?  (That is, why is it okay to ignore the "type" argument?)

On the other hand, you probably *should* be ignoring the "max"
argument, because AFAICS the values that are passed for that are
specific to the English ASCII spellings of the day and month names.
It seems very likely that they could be too small for some sets of
non-English names.

Related to that, the business with

+	mb_max = max * pg_encoding_max_length(encoding);
+	name_len = strlen(name);
+	name_len = name_len < mb_max ? name_len : mb_max;

seems wrong even if we assume that "max" is a sane limit on the number of
characters to consider.  This coding is likely to truncate a long "name"
string in the middle of a multibyte character, leading to bad-encoding
errors from later functions.

I also am confused by the "if (mb_max > max ...)" bit.  It looks to me
like that's an obscure way of writing "if (pg_encoding_max_length() > 1)",
which is a rather pointless check considering that the if() then goes on
to insist on encoding == PG_UTF8.

A bit further down, you have an "if (name_wlen > norm_wlen)"
condition that seems pretty fishy.  Is it really guaranteed that
unicode_normalize_kc cannot transform the string without shortening it?
I don't see that specified in its header comment, for sure.

Also, it's purely accidental if this:

	norm_name = (char *) palloc((norm_wlen + 1) * sizeof(pg_wchar));

allocates a long enough string for the conversion back to multibyte form,
because that output is not pg_wchars.  I think you want something more
like "norm_wlen * MAX_MULTIBYTE_CHAR_LEN + 1".  (I wonder whether we
need to be worrying about integer overflow in any of this.)

It seems like you could eliminate a lot of messiness by extending
localized_abbrev_days[] and related arrays by one element that's
always NULL.  That would waste, hmm, four 8-byte pointers on typical
machines --- but you're eating a lot more than 32 bytes of code to
pass around the array lengths, plus it's really ugly that the plain
and localized arrays are handled differently.

I don't think the way you're managing the "localized_names" variable
in DCH_from_char is very sensible.  The reader has to do a lot of
reverse engineering just to discover that the value is constant NULL
in most references, something that you've not helped by declaring
it outside the loop rather than inside.  I think you could drop
the variable and write constant NULL in most places, with something
like
	S_TM(n->suffix) ? localized_full_days : NULL
in those from_char_seq_search calls where it's relevant.

In general I'm displeased with the lack of attention to comments.
Notably, you added arguments to from_char_seq_search without updating
its header comment ... not that that comment is a great API spec, but
at the least you shouldn't be making it worse.  I think the bug I
complained of above is directly traceable to the lack of a clear spec
here for what "max" means, so failure to keep these comments up to
speed does have demonstrable bad consequences.

			regards, tom lane





view thread (66+ messages)  latest in thread

Message-ID: <31691.1579648824@sss.pgh.pa.us>
Permalink:  ../31691.1579648824@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/31691.1579648824@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, juanjo.santamaria@gmail.com, zaartur@gmail.com, tomas.vondra@2ndquadrant.com, alvherre@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
  Subject: Re: Allow to_date() and to_timestamp() to accept localized names
  In-Reply-To: <31691.1579648824@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