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 |
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 |