Re: regexp_match() returning text

From: Emre Hasegeli <emre(at)hasegeli(dot)com>
To: "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>
Subject: Re: regexp_match() returning text
Date: 2016-06-04 11:59:25
Message-ID: CAE2gYzwH1pawO9npA-BCX2TUdK+E0h1W91n=ixiAn2Rx_hBqLw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> The main problem being solved is the use of a SETOF result. I'm inclined to
> prefer that the final, single, result is still an array.

I have changed it like that. New patch attached.

> I've got a style issue with the information_schema - I like to call it
> useless-use-of-E'' - but that was there long before this patch...

We better not touch it.

> /* user mustn't specify 'g' for regexp_split */ - do we add "or
> regexp_match" or just removed the extraneous detail?

I don't think it would be a nice error message.

> There seems to be scope creep regarding "regexp_split_to_table" that I'm
> surprised to find. Related to that is the unexpected removal of the
> "force_glob" parameter to setup_regexp_matches. You took what was a single
> block of code and now duplicated it without any explanation in the commit
> message (a code comment wouldn't work for this kind of change). The change
> to flags from passing a pointer to text to passing in a pointer to a
> previously derived pg_re_flags makes more sense on its face, and it is
> apparently a non-public API, but again constitutes a refactoring that at
> least would ideally be a separate commit from the one the introduces the new
> behavior.

That check doesn't belong to setup_regexp_matches() in the first place.
The arguments of the function are organised to be caller agnostic,
and then it gives an error on behalf of regexp_split(). The check
fits better to the regexp_split() functions even with duplication.

I can split it to another patch, but I think these kind of changes most
often go together.

Would you mind adding yourself to the reviewers on the Commitfest app?
I think you have already read though it.

Thanks for all the feedback.

Attachment Content-Type Size
regexp-match-v2.patch text/x-diff 27.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-06-04 12:29:33 Re: [sqlsmith] Failed assertion in joinrels.c
Previous Message Amit Kapila 2016-06-04 06:57:20 Re: [sqlsmith] Failed assertions on parallel worker shutdown