Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Andres Freund and...@2ndquadrant.com writes: On 2014-05-09 22:14:25 +0900, Michael Paquier wrote: [ patch ] I've committed a revised version of Andres' patch. Mostly cosmetic fixes, but the hash implementation was still wrong: return DirectFunctionCall1(hashint8, PG_GETARG_LSN(0)); DirectFunctionCallN takes Datums, not de-Datumified values. This coding would've crashed on 32-bit machines, where hashint8 would be expecting to receive a Datum containing a pointer to int64. We could have done it correctly like this: return DirectFunctionCall1(hashint8, PG_GETARG_DATUM(0)); but I thought it was better, and microscopically more efficient, to follow the existing precedent in timestamp_hash: return hashint8(fcinfo); since that avoids an extra FunctionCallInfoData setup. On Fri, May 9, 2014 at 10:01 PM, Fujii Masao masao.fu...@gmail.com wrote: The patch looks good to me except the name of index operator class. I think that pg_lsn_ops is better than pglsn_ops because it's for pg_lsn data type. I concur, and changed this. I honestly don't really see much point in the added tests. If at all I'd rather see my tests from http://archives.postgresql.org/message-id/20140506230722.GE24808%40awork2.anarazel.de addded. With some EXPLAIN (COSTS OFF) they'd test more. I thought even that was kind of overkill; but a bigger problem is the output was sensitive to hash values which are not portable across different architectures. With a bit of experimentation I found that a SELECT DISTINCT ... ORDER BY query would exercise both hashing and sorting, so that's what got committed. (I'm not entirely sure though whether the plan will be stable across architectures; we might have to tweak the test case based on buildfarm 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
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Thu, Jun 5, 2014 at 9:54 AM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-05-09 22:14:25 +0900, Michael Paquier wrote: [ patch ] I've committed a revised version of Andres' patch. Thanks! I thought even that was kind of overkill; but a bigger problem is the output was sensitive to hash values which are not portable across different architectures. With a bit of experimentation I found that a SELECT DISTINCT ... ORDER BY query would exercise both hashing and sorting, so that's what got committed. (I'm not entirely sure though whether the plan will be stable across architectures; we might have to tweak the test case based on buildfarm feedback.) Yeah this was a bit too much, and came up with some more light-weight regression tests instead in the patch here: http://www.postgresql.org/message-id/CAB7nPqSgZVrHRgsgUg63SCFY+AwH-=3judy7moq-_fo7wi4...@mail.gmail.com -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sat, May 10, 2014 at 2:43 AM, Michael Paquier michael.paqu...@gmail.comwrote: On Fri, May 9, 2014 at 10:49 PM, Fujii Masao masao.fu...@gmail.com wrote: On Fri, May 9, 2014 at 10:03 PM, Andres Freund and...@2ndquadrant.com wrote: You plan to commit it? Yes unless many people object the commit. Michael, you're now modifying the patch? OK, I have been able to put my head into that earlier than I thought as it would be better to get that done before the beta, finishing with the attached. When hacking that, I noticed that some tests were missing for some of the operators. I am including them in this patch as well, with more tests for unique index creation, ordering, and hash/btree index creation. The test suite looks cleaner with all that. Last night I sent a patch [1] to add more tests and change the operator name. Maybe we can merge the test cases... ;-) Regards, [1] http://www.postgresql.org/message-id/cafcns+pmvil+x1kf3pumc2cp+e1juogbdmyz+se0plrr9tm...@mail.gmail.com -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sat, May 10, 2014 at 9:51 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Fri, May 9, 2014 at 9:30 PM, Tom Lane t...@sss.pgh.pa.us wrote: =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= fabriziome...@gmail.com writes: On Fri, May 9, 2014 at 8:42 PM, Tom Lane t...@sss.pgh.pa.us wrote: I think it's really too late for this for 9.4. At this point it's less than 48 hours until beta1 wraps, and we do not have the bandwidth to do anything but worry about stabilizing the features we've already got. But it's a very small change with many benefits, and Michael acted very proactive to make this happens. [ shrug... ] proactive would have been doing this a month ago. If we're going to ship a release, we have to stop taking new features at some point, and we are really past that point for 9.4. And, to be blunt, this is not important enough to hold up the release for, nor to take any stability risks for. It should go into the next commitfest cycle where it can get a non-rushed review. I agree with you that is too late to add *new features*. But I agree with Andres when he said this is a regression introcuced in the pg_lsn patch. So we'll release a version that break a simple query like that: fabrizio=# SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1, 100) g(i), generate_series(1, 5); ERROR: could not identify an equality operator for type pg_lsn LINE 1: SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1... ^ I agree that this is not new feature but just the fix of oversight of the pg_lsn patch. Without such opclass, we cannot execute even such simple query. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 2014-05-11 06:02:23 +0900, Fujii Masao wrote: On Sat, May 10, 2014 at 9:51 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Fri, May 9, 2014 at 9:30 PM, Tom Lane t...@sss.pgh.pa.us wrote: [ shrug... ] proactive would have been doing this a month ago. If we're going to ship a release, we have to stop taking new features at some point, and we are really past that point for 9.4. We *couldn't* do it a month ago since we didn't know about it a month ago. My delorean is getting a bit old. I agree with you that is too late to add *new features*. But I agree with Andres when he said this is a regression introcuced in the pg_lsn patch. So we'll release a version that break a simple query like that: fabrizio=# SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1, 100) g(i), generate_series(1, 5); ERROR: could not identify an equality operator for type pg_lsn LINE 1: SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1... ^ I agree that this is not new feature but just the fix of oversight of the pg_lsn patch. Without such opclass, we cannot execute even such simple query. I don't even understand why it's questionable whether this should be fixed. It's an almost trivial patch for an oversight in a newly introduced feature. We gain absolutely nothing by patching this in the next cycle, except that things need to be tested twice. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Andres Freund and...@2ndquadrant.com writes: I don't even understand why it's questionable whether this should be fixed. Sigh. We have some debate isomorphic to this one every year, it seems like. The argument why it shouldn't be fixed now is: ITS. TOO. LATE. Which part of that isn't clear to you? Or, if you think that this feature is so important that we should slip the beta schedule to get it in, we can take a vote on that. But at this point any slip means no beta till after PGCon. 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
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sat, May 10, 2014 at 6:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: I don't even understand why it's questionable whether this should be fixed. Sigh. We have some debate isomorphic to this one every year, it seems like. The argument why it shouldn't be fixed now is: ITS. TOO. LATE. Which part of that isn't clear to you? Sorry but I don't understand why it's too late. The 9.4 branch not been created yet. And in the last hours various patches (mostly fixes) have been committed and IMO this is not different. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 2014-05-10 19:19:22 -0300, Fabrízio de Royes Mello wrote: On Sat, May 10, 2014 at 6:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: I don't even understand why it's questionable whether this should be fixed. Sigh. We have some debate isomorphic to this one every year, it seems like. The argument why it shouldn't be fixed now is: ITS. TOO. LATE. Which part of that isn't clear to you? Sorry but I don't understand why it's too late. The 9.4 branch not been created yet. The problem is that once the beta is in progress (starting tomorrow), the projects tries to avoid changes which require a dump and restore (or pg_upgrade). Since the patch changes the catalog it'd require that. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sun, May 11, 2014 at 12:27 AM, Andres Freund and...@2ndquadrant.comwrote: On 2014-05-10 19:19:22 -0300, Fabrízio de Royes Mello wrote: On Sat, May 10, 2014 at 6:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: I don't even understand why it's questionable whether this should be fixed. Sigh. We have some debate isomorphic to this one every year, it seems like. The argument why it shouldn't be fixed now is: ITS. TOO. LATE. Which part of that isn't clear to you? Sorry but I don't understand why it's too late. The 9.4 branch not been created yet. The problem is that once the beta is in progress (starting tomorrow), the projects tries to avoid changes which require a dump and restore (or pg_upgrade). Since the patch changes the catalog it'd require that. It would be pg_upgrade'able though, wouldn't it? Don't we have precedents for requiring pg_upgrade during beta? At least that's a smaller problem than requiring a complete dump/reload. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Andres Freund and...@2ndquadrant.com writes: On 2014-05-10 19:19:22 -0300, Fabrízio de Royes Mello wrote: Sorry but I don't understand why it's too late. The 9.4 branch not been created yet. The problem is that once the beta is in progress (starting tomorrow), the projects tries to avoid changes which require a dump and restore (or pg_upgrade). Since the patch changes the catalog it'd require that. Exactly. If this isn't in before beta1, it's not going in, unless perhaps we encounter some bug bad enough to force an initdb later in beta. Which is certainly possible, and if it did happen there might be room to reconsider. But the same policy means there is zero margin for error with a catalog-changing patch applied right before beta starts, which is what we're debating here. As a comparison point, in a nearby thread we're debating renaming jsonb_hash_ops, which is a sufficiently mechanical change that I'd not be too afraid to do it the day before beta. But there are enough ways to screw up a new operator class that I'm very afraid of putting one in this weekend. 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
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sat, May 10, 2014 at 7:27 PM, Andres Freund and...@2ndquadrant.com wrote: On 2014-05-10 19:19:22 -0300, Fabrízio de Royes Mello wrote: On Sat, May 10, 2014 at 6:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: I don't even understand why it's questionable whether this should be fixed. Sigh. We have some debate isomorphic to this one every year, it seems like. The argument why it shouldn't be fixed now is: ITS. TOO. LATE. Which part of that isn't clear to you? Sorry but I don't understand why it's too late. The 9.4 branch not been created yet. The problem is that once the beta is in progress (starting tomorrow), the projects tries to avoid changes which require a dump and restore (or pg_upgrade). Since the patch changes the catalog it'd require that. H... so the other option maybe is create an extension to add this operators. Thoughts? -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 2014-05-11 00:31:09 +0200, Magnus Hagander wrote: On Sun, May 11, 2014 at 12:27 AM, Andres Freund and...@2ndquadrant.comwrote: On 2014-05-10 19:19:22 -0300, Fabrízio de Royes Mello wrote: On Sat, May 10, 2014 at 6:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: I don't even understand why it's questionable whether this should be fixed. Sigh. We have some debate isomorphic to this one every year, it seems like. The argument why it shouldn't be fixed now is: ITS. TOO. LATE. Which part of that isn't clear to you? Sorry but I don't understand why it's too late. The 9.4 branch not been created yet. The problem is that once the beta is in progress (starting tomorrow), the projects tries to avoid changes which require a dump and restore (or pg_upgrade). Since the patch changes the catalog it'd require that. It would be pg_upgrade'able though, wouldn't it? Yes. Don't we have precedents for requiring pg_upgrade during beta? At least that's a smaller problem than requiring a complete dump/reload. I think in reality there's been catversion updates after most of the recent beta1 releases. 9.3 (one): git log 817a89423f429a6a8b36847ee499f5b6be39c3be.. -- src/include/catalog/catversion.h 9.2 (one, but reverted): git log f70fa835e08eee4cb2dc0f72d66cf633089c37e8..REL9_2_0 -- src/include/catalog/catversion.h 9.1 (two): git log 993c5e59047dd568d4831f7ec5c6199acd21f17f..REL9_1_0 -- src/include/catalog/catversion.h 9.0 (one) git log -p f9d9b2b34a094b94fda39231c16ab5f2e6bfbbe4..REL9_0_0 -- src/include/catalog/catversion.h (makes me really wish betas were properly tagged with git as well...) Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sun, May 11, 2014 at 7:39 AM, Andres Freund and...@2ndquadrant.com wrote: (makes me really wish betas were properly tagged with git as well...) They are tags for betas, here is for example the update of CATVERSION for 9.3: $ git log -p REL9_3_BETA1..REL9_3_0 src/include/catalog/catversion.h | grep commit commit dc3eb5638349e74a6628130a5101ce866455f4a3 -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Magnus Hagander mag...@hagander.net writes: On Sun, May 11, 2014 at 12:27 AM, Andres Freund and...@2ndquadrant.comwrote: The problem is that once the beta is in progress (starting tomorrow), the projects tries to avoid changes which require a dump and restore (or pg_upgrade). Since the patch changes the catalog it'd require that. It would be pg_upgrade'able though, wouldn't it? Don't we have precedents for requiring pg_upgrade during beta? At least that's a smaller problem than requiring a complete dump/reload. pg_upgrade makes the penalty for screwups smaller, but a post-beta1 initdb is still the result of a screwup. None of the historical examples you mention were planned in advance of beta. 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
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 2014-05-10 19:08:48 -0400, Tom Lane wrote: Magnus Hagander mag...@hagander.net writes: On Sun, May 11, 2014 at 12:27 AM, Andres Freund and...@2ndquadrant.comwrote: The problem is that once the beta is in progress (starting tomorrow), the projects tries to avoid changes which require a dump and restore (or pg_upgrade). Since the patch changes the catalog it'd require that. It would be pg_upgrade'able though, wouldn't it? Don't we have precedents for requiring pg_upgrade during beta? At least that's a smaller problem than requiring a complete dump/reload. pg_upgrade makes the penalty for screwups smaller, but a post-beta1 initdb is still the result of a screwup. None of the historical examples you mention were planned in advance of beta. Yea, I posted that just to answer Magnus' question. I've argued that this omission should be fixed since tuesday. There's been a tested and reviewed patch since 20140506230722.ge24...@awork2.anarazel.de. Given how many changes went in since it certainly wouldn't have been a very destabilizing commit. Anyway. I accept it's too late for beta1 now. Let's commit it if there's another catversion bump. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sat, May 10, 2014 at 9:45 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: Last night I sent a patch [1] to add more tests and change the operator name. Maybe we can merge the test cases... ;-) Sure, I noticed that. But I think that they are more complicated than necessary. I am as well doubtful about adding test cases with EXPLAIN for a data type test suite. The patch introduces two new things: pg_lsn_cmp and pg_lsn_hash. To test the former a simple ORDER BY query is enough as cmp is used as an ordering operator. And for the latter creating a hash index looks enough as it tests using the hash function when inserting index tuples. Not to mention as well that the tests are more readable. Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Sun, May 11, 2014 at 8:17 AM, Andres Freund and...@2ndquadrant.com wrote: Anyway. I accept it's too late for beta1 now. Let's commit it if there's another catversion bump. +1. Let's rely on the experience of senior committers here. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Wed, May 7, 2014 at 11:55 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Tue, May 6, 2014 at 8:25 PM, Michael Paquier michael.paqu...@gmail.com
wrote:
On Wed, May 7, 2014 at 8:22 AM, Andres Freund and...@2ndquadrant.com
wrote:
Uh. They're different:
Datum
timestamp_hash(PG_FUNCTION_ARGS)
{
/* We can use either hashint8 or hashfloat8 directly */
#ifdef HAVE_INT64_TIMESTAMP
return hashint8(fcinfo);
#else
return hashfloat8(fcinfo);
#endif
}
note it's passing fcinfo, not the datum as you do. Same with
time_hash.. In fact your version crashes when used because it's
dereferencing a int8 as a pointer inside hashfloat8.
Thanks, didn't notice that fcinfo was used.
Hi all,
If helps, I added some regression tests to the lastest patch.
+DATA(insert OID = 3260 (403pglsn_opsPGNSP PGUID ));
+DATA(insert OID = 3261 (405pglsn_opsPGNSP PGUID ));
The patch looks good to me except the name of index operator class.
I think that pg_lsn_ops is better than pglsn_ops because it's for pg_lsn
data type.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 2014-05-09 22:01:07 +0900, Fujii Masao wrote:
On Wed, May 7, 2014 at 11:55 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Tue, May 6, 2014 at 8:25 PM, Michael Paquier michael.paqu...@gmail.com
wrote:
On Wed, May 7, 2014 at 8:22 AM, Andres Freund and...@2ndquadrant.com
wrote:
Uh. They're different:
Datum
timestamp_hash(PG_FUNCTION_ARGS)
{
/* We can use either hashint8 or hashfloat8 directly */
#ifdef HAVE_INT64_TIMESTAMP
return hashint8(fcinfo);
#else
return hashfloat8(fcinfo);
#endif
}
note it's passing fcinfo, not the datum as you do. Same with
time_hash.. In fact your version crashes when used because it's
dereferencing a int8 as a pointer inside hashfloat8.
Thanks, didn't notice that fcinfo was used.
Hi all,
If helps, I added some regression tests to the lastest patch.
+DATA(insert OID = 3260 (403pglsn_opsPGNSP PGUID ));
+DATA(insert OID = 3261 (405pglsn_opsPGNSP PGUID ));
The patch looks good to me except the name of index operator class.
FWIW, I've tested and looked through the patch as well.
I think that pg_lsn_ops is better than pglsn_ops because it's for pg_lsn
data type.
You're right, that's marginally prettier.
You plan to commit it?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Fri, May 9, 2014 at 10:01 PM, Fujii Masao masao.fu...@gmail.com wrote: +DATA(insert OID = 3260 (403pglsn_opsPGNSP PGUID )); +DATA(insert OID = 3261 (405pglsn_opsPGNSP PGUID )); The patch looks good to me except the name of index operator class. I think that pg_lsn_ops is better than pglsn_ops because it's for pg_lsn data type. Well, yes... Btw, before doing anything with this patch, I think that the version of Fabrizio could be used as a base, but the regression tests should be reshuffled a bit... -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 2014-05-09 22:14:25 +0900, Michael Paquier wrote: On Fri, May 9, 2014 at 10:01 PM, Fujii Masao masao.fu...@gmail.com wrote: +DATA(insert OID = 3260 (403pglsn_opsPGNSP PGUID )); +DATA(insert OID = 3261 (405pglsn_opsPGNSP PGUID )); The patch looks good to me except the name of index operator class. I think that pg_lsn_ops is better than pglsn_ops because it's for pg_lsn data type. Well, yes... Btw, before doing anything with this patch, I think that the version of Fabrizio could be used as a base, but the regression tests should be reshuffled a bit... I honestly don't really see much point in the added tests. If at all I'd rather see my tests from http://archives.postgresql.org/message-id/20140506230722.GE24808%40awork2.anarazel.de addded. With some EXPLAIN (COSTS OFF) they'd test more. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Fri, May 9, 2014 at 10:03 PM, Andres Freund and...@2ndquadrant.com wrote:
On 2014-05-09 22:01:07 +0900, Fujii Masao wrote:
On Wed, May 7, 2014 at 11:55 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Tue, May 6, 2014 at 8:25 PM, Michael Paquier michael.paqu...@gmail.com
wrote:
On Wed, May 7, 2014 at 8:22 AM, Andres Freund and...@2ndquadrant.com
wrote:
Uh. They're different:
Datum
timestamp_hash(PG_FUNCTION_ARGS)
{
/* We can use either hashint8 or hashfloat8 directly */
#ifdef HAVE_INT64_TIMESTAMP
return hashint8(fcinfo);
#else
return hashfloat8(fcinfo);
#endif
}
note it's passing fcinfo, not the datum as you do. Same with
time_hash.. In fact your version crashes when used because it's
dereferencing a int8 as a pointer inside hashfloat8.
Thanks, didn't notice that fcinfo was used.
Hi all,
If helps, I added some regression tests to the lastest patch.
+DATA(insert OID = 3260 (403pglsn_opsPGNSP PGUID ));
+DATA(insert OID = 3261 (405pglsn_opsPGNSP PGUID ));
The patch looks good to me except the name of index operator class.
FWIW, I've tested and looked through the patch as well.
I think that pg_lsn_ops is better than pglsn_ops because it's for
pg_lsn
data type.
You're right, that's marginally prettier.
You plan to commit it?
Yes unless many people object the commit.
Michael,
You're now modifying the patch?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Fri, May 9, 2014 at 10:49 PM, Fujii Masao masao.fu...@gmail.com wrote: Yes unless many people object the commit. Michael, You're now modifying the patch? Not within a couple of days. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Michael Paquier michael.paqu...@gmail.com writes: On Fri, May 9, 2014 at 10:49 PM, Fujii Masao masao.fu...@gmail.com wrote: Yes unless many people object the commit. Michael, You're now modifying the patch? Not within a couple of days. I think it's really too late for this for 9.4. At this point it's less than 48 hours until beta1 wraps, and we do not have the bandwidth to do anything but worry about stabilizing the features we've already got. 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
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Fri, May 9, 2014 at 10:18 AM, Andres Freund and...@2ndquadrant.com wrote: On 2014-05-09 22:14:25 +0900, Michael Paquier wrote: On Fri, May 9, 2014 at 10:01 PM, Fujii Masao masao.fu...@gmail.com wrote: +DATA(insert OID = 3260 (403pglsn_opsPGNSP PGUID )); +DATA(insert OID = 3261 (405pglsn_opsPGNSP PGUID )); The patch looks good to me except the name of index operator class. I think that pg_lsn_ops is better than pglsn_ops because it's for pg_lsn data type. Well, yes... Btw, before doing anything with this patch, I think that the version of Fabrizio could be used as a base, but the regression tests should be reshuffled a bit... I honestly don't really see much point in the added tests. If at all I'd rather see my tests from http://archives.postgresql.org/message-id/20140506230722.GE24808%40awork2.anarazel.de addded. With some EXPLAIN (COSTS OFF) they'd test more. Ok. In a few hours I'll add your suggestion and send it again. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Fri, May 9, 2014 at 8:42 PM, Tom Lane t...@sss.pgh.pa.us wrote: Michael Paquier michael.paqu...@gmail.com writes: On Fri, May 9, 2014 at 10:49 PM, Fujii Masao masao.fu...@gmail.com wrote: Yes unless many people object the commit. Michael, You're now modifying the patch? Not within a couple of days. I think it's really too late for this for 9.4. At this point it's less than 48 hours until beta1 wraps, and we do not have the bandwidth to do anything but worry about stabilizing the features we've already got. But it's a very small change with many benefits, and Michael acted very proactive to make this happens. In a few hours I'll fix the last two suggestions (Fujii and Andres) and send it again. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= fabriziome...@gmail.com writes: On Fri, May 9, 2014 at 8:42 PM, Tom Lane t...@sss.pgh.pa.us wrote: I think it's really too late for this for 9.4. At this point it's less than 48 hours until beta1 wraps, and we do not have the bandwidth to do anything but worry about stabilizing the features we've already got. But it's a very small change with many benefits, and Michael acted very proactive to make this happens. [ shrug... ] proactive would have been doing this a month ago. If we're going to ship a release, we have to stop taking new features at some point, and we are really past that point for 9.4. And, to be blunt, this is not important enough to hold up the release for, nor to take any stability risks for. It should go into the next commitfest cycle where it can get a non-rushed review. 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
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Fri, May 9, 2014 at 9:30 PM, Tom Lane t...@sss.pgh.pa.us wrote:
=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= fabriziome...@gmail.com writes:
On Fri, May 9, 2014 at 8:42 PM, Tom Lane t...@sss.pgh.pa.us wrote:
I think it's really too late for this for 9.4. At this point it's
less than 48 hours until beta1 wraps, and we do not have the bandwidth
to do anything but worry about stabilizing the features we've already
got.
But it's a very small change with many benefits, and Michael acted very
proactive to make this happens.
[ shrug... ] proactive would have been doing this a month ago.
If we're going to ship a release, we have to stop taking new features
at some point, and we are really past that point for 9.4.
And, to be blunt, this is not important enough to hold up the release
for, nor to take any stability risks for. It should go into the next
commitfest cycle where it can get a non-rushed review.
I agree with you that is too late to add *new features*.
But I agree with Andres when he said this is a regression introcuced in the
pg_lsn patch.
So we'll release a version that break a simple query like that:
fabrizio=# SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1,
100) g(i), generate_series(1, 5);
ERROR: could not identify an equality operator for type pg_lsn
LINE 1: SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1...
^
I attached the last version of this fix with the Andres and Fujii
suggestions.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Timbira: http://www.timbira.com.br
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index d1448ae..c021a1e 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -14,6 +14,7 @@
#include postgres.h
#include funcapi.h
+#include access/hash.h
#include libpq/pqformat.h
#include utils/builtins.h
#include utils/pg_lsn.h
@@ -153,6 +154,27 @@ pg_lsn_ge(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(lsn1 = lsn2);
}
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr a = PG_GETARG_LSN(0);
+ XLogRecPtr b = PG_GETARG_LSN(1);
+
+ if (a b)
+ PG_RETURN_INT32(1);
+ else if (a == b)
+ PG_RETURN_INT32(0);
+ else
+ PG_RETURN_INT32(-1);
+}
+
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ return DirectFunctionCall1(hashint8, PG_GETARG_LSN(0));
+}
/*--
* Arithmetic operators on PostgreSQL LSNs.
diff --git a/src/include/catalog/pg_amop.h b/src/include/catalog/pg_amop.h
index 8efd3be..35fd17e 100644
--- a/src/include/catalog/pg_amop.h
+++ b/src/include/catalog/pg_amop.h
@@ -513,6 +513,16 @@ DATA(insert ( 2968 2950 2950 4 s 2977 403 0 ));
DATA(insert ( 2968 2950 2950 5 s 2975 403 0 ));
/*
+ * btree pg_lsn_ops
+ */
+
+DATA(insert ( 3260 3220 3220 1 s 3224 403 0 ));
+DATA(insert ( 3260 3220 3220 2 s 3226 403 0 ));
+DATA(insert ( 3260 3220 3220 3 s 3222 403 0 ));
+DATA(insert ( 3260 3220 3220 4 s 3227 403 0 ));
+DATA(insert ( 3260 3220 3220 5 s 3225 403 0 ));
+
+/*
* hash index _ops
*/
@@ -581,6 +591,8 @@ DATA(insert ( 2231 1042 1042 1 s 1054 405 0 ));
DATA(insert ( 2235 1033 1033 1 s 974 405 0 ));
/* uuid_ops */
DATA(insert ( 2969 2950 2950 1 s 2972 405 0 ));
+/* pg_lsn_ops */
+DATA(insert ( 3261 3220 3220 1 s 3222 405 0 ));
/* numeric_ops */
DATA(insert ( 1998 1700 1700 1 s 1752 405 0 ));
/* array_ops */
diff --git a/src/include/catalog/pg_amproc.h b/src/include/catalog/pg_amproc.h
index 198b126..369adb9 100644
--- a/src/include/catalog/pg_amproc.h
+++ b/src/include/catalog/pg_amproc.h
@@ -134,6 +134,7 @@ DATA(insert ( 2789 27 27 1 2794 ));
DATA(insert ( 2968 2950 2950 1 2960 ));
DATA(insert ( 2994 2249 2249 1 2987 ));
DATA(insert ( 3194 2249 2249 1 3187 ));
+DATA(insert ( 3260 3220 3220 1 3263 ));
DATA(insert ( 3522 3500 3500 1 3514 ));
DATA(insert ( 3626 3614 3614 1 3622 ));
DATA(insert ( 3683 3615 3615 1 3668 ));
@@ -174,6 +175,7 @@ DATA(insert ( 2229 25 25 1 400 ));
DATA(insert ( 2231 1042 1042 1 1080 ));
DATA(insert ( 2235 1033 1033 1 329 ));
DATA(insert ( 2969 2950 2950 1 2963 ));
+DATA(insert ( 3261 3220 3220 1 3262 ));
DATA(insert ( 3523 3500 3500 1 3515 ));
DATA(insert ( 3903 3831 3831 1 3902 ));
DATA(insert ( 4034 3802 3802 1 4045 ));
diff --git a/src/include/catalog/pg_opclass.h b/src/include/catalog/pg_opclass.h
index ecf7063..2298a83 100644
--- a/src/include/catalog/pg_opclass.h
+++ b/src/include/catalog/pg_opclass.h
@@ -215,6 +215,8 @@ DATA(insert ( 2742 _reltime_ops PGNSP PGUID 2745 1024 t 703 ));
DATA(insert ( 2742 _tinterval_ops PGNSP PGUID 2745 1025 t 704 ));
DATA(insert ( 403 uuid_ops PGNSP PGUID 2968 2950 t 0 ));
DATA(insert ( 405 uuid_ops PGNSP
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Fri, May 9, 2014 at 10:49 PM, Fujii Masao masao.fu...@gmail.com wrote:
On Fri, May 9, 2014 at 10:03 PM, Andres Freund and...@2ndquadrant.com wrote:
You plan to commit it?
Yes unless many people object the commit.
Michael, you're now modifying the patch?
OK, I have been able to put my head into that earlier than I thought
as it would be better to get that done before the beta, finishing with
the attached. When hacking that, I noticed that some tests were
missing for some of the operators. I am including them in this patch
as well, with more tests for unique index creation, ordering, and
hash/btree index creation. The test suite looks cleaner with all that.
--
Michael
From 13513f3392085edb0b3a75af12ef9d08334bdb2e Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Sat, 10 May 2014 14:34:04 +0900
Subject: [PATCH] Add hash/btree operators for pg_lsn
At the same time, regression tests are completed with some tests for
operators that were missing and of course new tests for the new btree
and hash operators.
---
src/backend/utils/adt/pg_lsn.c | 21
src/include/catalog/pg_amop.h| 12 +
src/include/catalog/pg_amproc.h | 2 +
src/include/catalog/pg_opclass.h | 2 +
src/include/catalog/pg_operator.h| 2 +-
src/include/catalog/pg_opfamily.h| 2 +
src/include/catalog/pg_proc.h| 4 ++
src/include/utils/pg_lsn.h | 2 +
src/test/regress/expected/pg_lsn.out | 96 ++--
src/test/regress/sql/pg_lsn.sql | 46 +
10 files changed, 165 insertions(+), 24 deletions(-)
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index d1448ae..b779907 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -14,6 +14,7 @@
#include postgres.h
#include funcapi.h
+#include access/hash.h
#include libpq/pqformat.h
#include utils/builtins.h
#include utils/pg_lsn.h
@@ -153,6 +154,26 @@ pg_lsn_ge(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(lsn1 = lsn2);
}
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr a = PG_GETARG_LSN(0);
+ XLogRecPtr b = PG_GETARG_LSN(1);
+
+ if (a b)
+ PG_RETURN_INT32(-1);
+ else if (a b)
+ PG_RETURN_INT32(1);
+ PG_RETURN_INT32(0);
+}
+
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ return DirectFunctionCall1(hashint8, PG_GETARG_LSN(0));
+}
/*--
* Arithmetic operators on PostgreSQL LSNs.
diff --git a/src/include/catalog/pg_amop.h b/src/include/catalog/pg_amop.h
index 8efd3be..35fd17e 100644
--- a/src/include/catalog/pg_amop.h
+++ b/src/include/catalog/pg_amop.h
@@ -513,6 +513,16 @@ DATA(insert ( 2968 2950 2950 4 s 2977403 0
));
DATA(insert ( 2968 2950 2950 5 s 2975403 0 ));
/*
+ * btree pg_lsn_ops
+ */
+
+DATA(insert ( 3260 3220 3220 1 s 3224403 0 ));
+DATA(insert ( 3260 3220 3220 2 s 3226403 0 ));
+DATA(insert ( 3260 3220 3220 3 s 3222403 0 ));
+DATA(insert ( 3260 3220 3220 4 s 3227403 0 ));
+DATA(insert ( 3260 3220 3220 5 s 3225403 0 ));
+
+/*
* hash index _ops
*/
@@ -581,6 +591,8 @@ DATA(insert ( 2231 1042 1042 1 s 1054 405 0 ));
DATA(insert ( 2235 1033 1033 1 s 974 405 0 ));
/* uuid_ops */
DATA(insert ( 2969 2950 2950 1 s 2972 405 0 ));
+/* pg_lsn_ops */
+DATA(insert ( 3261 3220 3220 1 s 3222 405 0 ));
/* numeric_ops */
DATA(insert ( 1998 1700 1700 1 s 1752 405 0 ));
/* array_ops */
diff --git a/src/include/catalog/pg_amproc.h b/src/include/catalog/pg_amproc.h
index 198b126..369adb9 100644
--- a/src/include/catalog/pg_amproc.h
+++ b/src/include/catalog/pg_amproc.h
@@ -134,6 +134,7 @@ DATA(insert ( 2789 27 27 1 2794 ));
DATA(insert ( 2968 2950 2950 1 2960 ));
DATA(insert ( 2994 2249 2249 1 2987 ));
DATA(insert ( 3194 2249 2249 1 3187 ));
+DATA(insert ( 3260 3220 3220 1 3263 ));
DATA(insert ( 3522 3500 3500 1 3514 ));
DATA(insert ( 3626 3614 3614 1 3622 ));
DATA(insert ( 3683 3615 3615 1 3668 ));
@@ -174,6 +175,7 @@ DATA(insert ( 2229 25 25 1 400 ));
DATA(insert ( 2231 1042 1042 1 1080 ));
DATA(insert ( 2235 1033 1033 1 329 ));
DATA(insert ( 2969 2950 2950 1 2963 ));
+DATA(insert ( 3261 3220 3220 1 3262 ));
DATA(insert ( 3523 3500 3500 1 3515 ));
DATA(insert ( 3903 3831 3831 1 3902 ));
DATA(insert ( 4034 3802 3802 1 4045 ));
diff --git a/src/include/catalog/pg_opclass.h b/src/include/catalog/pg_opclass.h
index ecf7063..2298a83 100644
--- a/src/include/catalog/pg_opclass.h
+++ b/src/include/catalog/pg_opclass.h
@@ -215,6 +215,8 @@ DATA(insert ( 2742_reltime_opsPGNSP
PGUID 2745 1024 t 703 ));
DATA(insert ( 2742_tinterval_ops PGNSP PGUID 2745 1025 t 704 ));
DATA(insert ( 403 uuid_ops
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Hi, On 2014-05-06 23:55:04 -0300, Fabrízio de Royes Mello wrote: If helps, I added some regression tests to the lastest patch. I think we should apply this patch now. It's much more sensible with the opclasses present and we don't win anything by waiting for 9.5. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Wed, May 7, 2014 at 1:21 PM, Craig Ringer cr...@2ndquadrant.com wrote: On 05/07/2014 07:16 AM, Michael Paquier wrote: On Wed, May 7, 2014 at 8:07 AM, Andres Freund and...@2ndquadrant.com wrote: On 2014-05-06 22:49:07 +0900, Michael Paquier wrote: FWIW, the format you're using makes applying the patch including the commit message relatively hard. Consider using git format-patch. Could you be clearer? By applying a filterdiff command or by using git diff? The latter has been used for this patch. git format-patch -1 is usually what you want to emit a patch for the top commit. To produce a patchset between the current branch and master: git format-patch master It doesn't use context diff, but I think people here have mostly stopped caring about that now (?). Thanks for the details, I didn't know this subcommand of git. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Andres Freund and...@2ndquadrant.com writes: Craig just mentioned in an internal chat that there's no btree or even hash opclass for the new pg_lsn type. That restricts what you can do with it quite severely. Imo this should be fixed for 9.4 - after all it was possible unto now to index a table with lsns returned by system functions or have queries with grouping on them without casting. Sorry, it is *way* too late for 9.4. 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
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 2014-05-06 09:37:54 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Craig just mentioned in an internal chat that there's no btree or even hash opclass for the new pg_lsn type. That restricts what you can do with it quite severely. Imo this should be fixed for 9.4 - after all it was possible unto now to index a table with lsns returned by system functions or have queries with grouping on them without casting. Sorry, it is *way* too late for 9.4. It's imo a regression/oversight introduced in the pg_lsn patch. Not a new feature. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Tue, May 6, 2014 at 4:14 PM, Andres Freund and...@2ndquadrant.com wrote:
Hi,
Craig just mentioned in an internal chat that there's no btree or even
hash opclass for the new pg_lsn type. That restricts what you can do
with it quite severely.
Imo this should be fixed for 9.4 - after all it was possible unto now to
index a table with lsns returned by system functions or have queries
with grouping on them without casting.
Makes sense, especially knowing operators needed for btree processing
are already defined. Patch attached solves that. Now to include it in
9.4 where development is over is another story... I wouldn't mind
adding it to the next commit fest either.
Regards,
--
Michael
commit 604177ac2af4bfd26a392b8669dd2fc3550f2a3d
Author: Michael Paquier mich...@otacoo.com
Date: Tue May 6 22:42:09 2014 +0900
Support for hash and btree operators for pg_lsn datatype
The following functions are added in pg_lsn bundle for the different
index operators:
- pg_lsn_hash for hash index
- pg_lsn_cmp for btree index
Related catalogs are updated as well.
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index e2b528a..f7980f8 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -150,6 +150,24 @@ pg_lsn_ge(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(lsn1 = lsn2);
}
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+ XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+ PG_RETURN_INT32(lsn1 == lsn2);
+}
+
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn = PG_GETARG_LSN(0);
+
+ return hashint8(lsn);
+}
/*--
* Arithmetic operators on PostgreSQL LSNs.
diff --git a/src/include/catalog/pg_amop.h b/src/include/catalog/pg_amop.h
index 8efd3be..5cd170d 100644
--- a/src/include/catalog/pg_amop.h
+++ b/src/include/catalog/pg_amop.h
@@ -513,6 +513,16 @@ DATA(insert ( 2968 2950 2950 4 s 2977403 0
));
DATA(insert ( 2968 2950 2950 5 s 2975403 0 ));
/*
+ * btree pglsn_ops
+ */
+
+DATA(insert ( 3260 3220 3220 1 s 3224403 0 ));
+DATA(insert ( 3260 3220 3220 2 s 3226403 0 ));
+DATA(insert ( 3260 3220 3220 3 s 3222403 0 ));
+DATA(insert ( 3260 3220 3220 4 s 3227403 0 ));
+DATA(insert ( 3260 3220 3220 5 s 3225403 0 ));
+
+/*
* hash index _ops
*/
@@ -581,6 +591,8 @@ DATA(insert ( 2231 1042 1042 1 s 1054 405 0 ));
DATA(insert ( 2235 1033 1033 1 s 974 405 0 ));
/* uuid_ops */
DATA(insert ( 2969 2950 2950 1 s 2972 405 0 ));
+/* pglsn_ops */
+DATA(insert ( 3261 3220 3220 1 s 3222 405 0 ));
/* numeric_ops */
DATA(insert ( 1998 1700 1700 1 s 1752 405 0 ));
/* array_ops */
diff --git a/src/include/catalog/pg_amproc.h b/src/include/catalog/pg_amproc.h
index 198b126..369adb9 100644
--- a/src/include/catalog/pg_amproc.h
+++ b/src/include/catalog/pg_amproc.h
@@ -134,6 +134,7 @@ DATA(insert ( 2789 27 27 1 2794 ));
DATA(insert ( 2968 2950 2950 1 2960 ));
DATA(insert ( 2994 2249 2249 1 2987 ));
DATA(insert ( 3194 2249 2249 1 3187 ));
+DATA(insert ( 3260 3220 3220 1 3263 ));
DATA(insert ( 3522 3500 3500 1 3514 ));
DATA(insert ( 3626 3614 3614 1 3622 ));
DATA(insert ( 3683 3615 3615 1 3668 ));
@@ -174,6 +175,7 @@ DATA(insert ( 2229 25 25 1 400 ));
DATA(insert ( 2231 1042 1042 1 1080 ));
DATA(insert ( 2235 1033 1033 1 329 ));
DATA(insert ( 2969 2950 2950 1 2963 ));
+DATA(insert ( 3261 3220 3220 1 3262 ));
DATA(insert ( 3523 3500 3500 1 3515 ));
DATA(insert ( 3903 3831 3831 1 3902 ));
DATA(insert ( 4034 3802 3802 1 4045 ));
diff --git a/src/include/catalog/pg_opclass.h b/src/include/catalog/pg_opclass.h
index 49b2410..cbcdacd 100644
--- a/src/include/catalog/pg_opclass.h
+++ b/src/include/catalog/pg_opclass.h
@@ -215,6 +215,8 @@ DATA(insert ( 2742_reltime_opsPGNSP
PGUID 2745 1024 t 703 ));
DATA(insert ( 2742_tinterval_ops PGNSP PGUID 2745 1025 t 704 ));
DATA(insert ( 403 uuid_opsPGNSP PGUID
2968 2950 t 0 ));
DATA(insert ( 405 uuid_opsPGNSP PGUID
2969 2950 t 0 ));
+DATA(insert ( 403 pglsn_ops PGNSP PGUID
3260 3220 t 0 ));
+DATA(insert ( 405 pglsn_ops PGNSP PGUID
3261 3220 t 0 ));
DATA(insert ( 403 enum_opsPGNSP PGUID
3522 3500 t 0 ));
DATA(insert ( 405 enum_opsPGNSP PGUID
3523 3500 t 0 ));
DATA(insert ( 403 tsvector_opsPGNSP PGUID 3626 3614
t 0 ));
diff --git a/src/include/catalog/pg_operator.h
b/src/include/catalog/pg_operator.h
index f280af4..87ee4eb 100644
--- a/src/include/catalog/pg_operator.h
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Andres Freund and...@2ndquadrant.com writes: On 2014-05-06 09:37:54 -0400, Tom Lane wrote: Sorry, it is *way* too late for 9.4. It's imo a regression/oversight introduced in the pg_lsn patch. Not a new feature. You can argue that if you like, but it doesn't matter. It's too late for a change as big as that for such an inessential feature. We are in the stabilization game at this point, and adding features is not the thing to be doing. Especially not features for which no patch even exists. I don't care if it could be done in a few hours by copy-and-paste, it would still be the very definition of rushed coding. We're past the window for this kind of change in 9.4. 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
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 05/06/2014 05:17 PM, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-05-06 09:37:54 -0400, Tom Lane wrote: Sorry, it is *way* too late for 9.4. It's imo a regression/oversight introduced in the pg_lsn patch. Not a new feature. You can argue that if you like, but it doesn't matter. It's too late for a change as big as that for such an inessential feature. We are in the stabilization game at this point, and adding features is not the thing to be doing. FWIW, I agree with Andres that this would be a reasonable thing to add. Exactly the kind of oversight that we should be fixing at this stage in the release cycle. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 05/06/2014 04:59 PM, Heikki Linnakangas wrote: On 05/06/2014 05:17 PM, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-05-06 09:37:54 -0400, Tom Lane wrote: Sorry, it is *way* too late for 9.4. It's imo a regression/oversight introduced in the pg_lsn patch. Not a new feature. You can argue that if you like, but it doesn't matter. It's too late for a change as big as that for such an inessential feature. We are in the stabilization game at this point, and adding features is not the thing to be doing. FWIW, I agree with Andres that this would be a reasonable thing to add. Exactly the kind of oversight that we should be fixing at this stage in the release cycle. I agree as well. -- Vik -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Tue, May 6, 2014 at 10:49 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
Makes sense, especially knowing operators needed for btree processing
are already defined. Patch attached solves that.
Please find attached an updated patch, I completely forgot that the
compare function needs to return {-1, 0, 1}.
--
Michael
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index e2b528a..baf789a 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -150,6 +150,28 @@ pg_lsn_ge(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(lsn1 = lsn2);
}
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+ XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+ if (lsn1 lsn2)
+ PG_RETURN_INT32(-1);
+ if (lsn1 lsn2)
+ PG_RETURN_INT32(1);
+ PG_RETURN_INT32(0);
+}
+
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn = PG_GETARG_LSN(0);
+
+ return hashint8(lsn);
+}
/*--
* Arithmetic operators on PostgreSQL LSNs.
diff --git a/src/include/catalog/pg_amop.h b/src/include/catalog/pg_amop.h
index 8efd3be..5cd170d 100644
--- a/src/include/catalog/pg_amop.h
+++ b/src/include/catalog/pg_amop.h
@@ -513,6 +513,16 @@ DATA(insert ( 2968 2950 2950 4 s 2977403 0
));
DATA(insert ( 2968 2950 2950 5 s 2975403 0 ));
/*
+ * btree pglsn_ops
+ */
+
+DATA(insert ( 3260 3220 3220 1 s 3224403 0 ));
+DATA(insert ( 3260 3220 3220 2 s 3226403 0 ));
+DATA(insert ( 3260 3220 3220 3 s 3222403 0 ));
+DATA(insert ( 3260 3220 3220 4 s 3227403 0 ));
+DATA(insert ( 3260 3220 3220 5 s 3225403 0 ));
+
+/*
* hash index _ops
*/
@@ -581,6 +591,8 @@ DATA(insert ( 2231 1042 1042 1 s 1054 405 0 ));
DATA(insert ( 2235 1033 1033 1 s 974 405 0 ));
/* uuid_ops */
DATA(insert ( 2969 2950 2950 1 s 2972 405 0 ));
+/* pglsn_ops */
+DATA(insert ( 3261 3220 3220 1 s 3222 405 0 ));
/* numeric_ops */
DATA(insert ( 1998 1700 1700 1 s 1752 405 0 ));
/* array_ops */
diff --git a/src/include/catalog/pg_amproc.h b/src/include/catalog/pg_amproc.h
index 198b126..369adb9 100644
--- a/src/include/catalog/pg_amproc.h
+++ b/src/include/catalog/pg_amproc.h
@@ -134,6 +134,7 @@ DATA(insert ( 2789 27 27 1 2794 ));
DATA(insert ( 2968 2950 2950 1 2960 ));
DATA(insert ( 2994 2249 2249 1 2987 ));
DATA(insert ( 3194 2249 2249 1 3187 ));
+DATA(insert ( 3260 3220 3220 1 3263 ));
DATA(insert ( 3522 3500 3500 1 3514 ));
DATA(insert ( 3626 3614 3614 1 3622 ));
DATA(insert ( 3683 3615 3615 1 3668 ));
@@ -174,6 +175,7 @@ DATA(insert ( 2229 25 25 1 400 ));
DATA(insert ( 2231 1042 1042 1 1080 ));
DATA(insert ( 2235 1033 1033 1 329 ));
DATA(insert ( 2969 2950 2950 1 2963 ));
+DATA(insert ( 3261 3220 3220 1 3262 ));
DATA(insert ( 3523 3500 3500 1 3515 ));
DATA(insert ( 3903 3831 3831 1 3902 ));
DATA(insert ( 4034 3802 3802 1 4045 ));
diff --git a/src/include/catalog/pg_opclass.h b/src/include/catalog/pg_opclass.h
index 49b2410..cbcdacd 100644
--- a/src/include/catalog/pg_opclass.h
+++ b/src/include/catalog/pg_opclass.h
@@ -215,6 +215,8 @@ DATA(insert ( 2742_reltime_opsPGNSP
PGUID 2745 1024 t 703 ));
DATA(insert ( 2742_tinterval_ops PGNSP PGUID 2745 1025 t 704 ));
DATA(insert ( 403 uuid_opsPGNSP PGUID
2968 2950 t 0 ));
DATA(insert ( 405 uuid_opsPGNSP PGUID
2969 2950 t 0 ));
+DATA(insert ( 403 pglsn_ops PGNSP PGUID
3260 3220 t 0 ));
+DATA(insert ( 405 pglsn_ops PGNSP PGUID
3261 3220 t 0 ));
DATA(insert ( 403 enum_opsPGNSP PGUID
3522 3500 t 0 ));
DATA(insert ( 405 enum_opsPGNSP PGUID
3523 3500 t 0 ));
DATA(insert ( 403 tsvector_opsPGNSP PGUID 3626 3614
t 0 ));
diff --git a/src/include/catalog/pg_operator.h
b/src/include/catalog/pg_operator.h
index f280af4..87ee4eb 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -1595,7 +1595,7 @@ DATA(insert OID = 2977 ( = PGNSP PGUID b f f
2950 2950 16 2976 2974 uuid_
DESCR(greater than or equal);
/* pg_lsn operators */
-DATA(insert OID = 3222 ( = PGNSP PGUID b f f 3220 3220 16 3222 3223
pg_lsn_eq eqsel eqjoinsel ));
+DATA(insert OID = 3222 ( = PGNSP PGUID b t t 3220 3220 16 3222 3223
pg_lsn_eq eqsel eqjoinsel ));
DESCR(equal);
DATA(insert OID = 3223 ( PGNSP PGUID b f f 3220 3220 16 3223 3222
pg_lsn_ne neqsel neqjoinsel ));
DESCR(not equal);
diff --git a/src/include/catalog/pg_opfamily.h
b/src/include/catalog/pg_opfamily.h
index 9e8f4ac..29089d5 100644
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
Hi,
On 2014-05-06 22:49:07 +0900, Michael Paquier wrote:
Makes sense, especially knowing operators needed for btree processing
are already defined. Patch attached solves that.
Thanks for doing that quickly.
FWIW, the format you're using makes applying the patch including the
commit message relatively hard. Consider using git format-patch.
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+ XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+ PG_RETURN_INT32(lsn1 == lsn2);
+}
This doesn't look correct. A cmp routine needs to return -1, 0, 1 when a
b, a = b, a b respectively. You'll only return 0 and 1 here.
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn = PG_GETARG_LSN(0);
+
+ return hashint8(lsn);
+}
That can't be right either. There's at least two things wrong here:
a) hashint8 takes PG_FUNCTION_ARGS, not a Datum
b) you're not including the necessary header defining hashint8.
I've used
SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1, 100) g(i),
generate_series(1, 5);
SELECT (g.i||'/0')::pg_lsn f FROM generate_series(1, 100) g(i) ORDER BY f;
SELECT (g.i||'/0')::pg_lsn, count(*) FROM generate_series(1, 100) g(i),
generate_series(1, 5) GROUP BY 1 ORDER BY 1;
SELECT * FROM
(SELECT (g.i||'/0')::pg_lsn lsn FROM generate_series(1, 10) g(i) ORDER BY
g.i) a
JOIN (SELECT (g.i||'/0')::pg_lsn lsn FROM generate_series(1, 10) g(i) ORDER
BY g.i DESC) b
ON (a.lsn = b.lsn );
To check that a) hashing works, b) sorting works, c) mergesorts works
after fixing the above issues. New version of the patch attached
I completely agree with Heikki's point that this kind of oversight is
the actual reason for a beta period.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From b25a8f57cb71389bbe823b3c9e2f13440176aad9 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 7 May 2014 01:05:57 +0200
Subject: [PATCH] Support for hash and btree operators for pg_lsn datatype.
Michael Paquier with fixes from Andres Freund.
---
src/backend/utils/adt/pg_lsn.c| 22 ++
src/include/catalog/pg_amop.h | 12
src/include/catalog/pg_amproc.h | 2 ++
src/include/catalog/pg_opclass.h | 2 ++
src/include/catalog/pg_operator.h | 2 +-
src/include/catalog/pg_opfamily.h | 2 ++
src/include/catalog/pg_proc.h | 4
src/include/utils/pg_lsn.h| 2 ++
8 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index d1448ae..c021a1e 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -14,6 +14,7 @@
#include postgres.h
#include funcapi.h
+#include access/hash.h
#include libpq/pqformat.h
#include utils/builtins.h
#include utils/pg_lsn.h
@@ -153,6 +154,27 @@ pg_lsn_ge(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(lsn1 = lsn2);
}
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr a = PG_GETARG_LSN(0);
+ XLogRecPtr b = PG_GETARG_LSN(1);
+
+ if (a b)
+ PG_RETURN_INT32(1);
+ else if (a == b)
+ PG_RETURN_INT32(0);
+ else
+ PG_RETURN_INT32(-1);
+}
+
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ return DirectFunctionCall1(hashint8, PG_GETARG_LSN(0));
+}
/*--
* Arithmetic operators on PostgreSQL LSNs.
diff --git a/src/include/catalog/pg_amop.h b/src/include/catalog/pg_amop.h
index 8efd3be..5cd170d 100644
--- a/src/include/catalog/pg_amop.h
+++ b/src/include/catalog/pg_amop.h
@@ -513,6 +513,16 @@ DATA(insert ( 2968 2950 2950 4 s 2977 403 0 ));
DATA(insert ( 2968 2950 2950 5 s 2975 403 0 ));
/*
+ * btree pglsn_ops
+ */
+
+DATA(insert ( 3260 3220 3220 1 s 3224 403 0 ));
+DATA(insert ( 3260 3220 3220 2 s 3226 403 0 ));
+DATA(insert ( 3260 3220 3220 3 s 3222 403 0 ));
+DATA(insert ( 3260 3220 3220 4 s 3227 403 0 ));
+DATA(insert ( 3260 3220 3220 5 s 3225 403 0 ));
+
+/*
* hash index _ops
*/
@@ -581,6 +591,8 @@ DATA(insert ( 2231 1042 1042 1 s 1054 405 0 ));
DATA(insert ( 2235 1033 1033 1 s 974 405 0 ));
/* uuid_ops */
DATA(insert ( 2969 2950 2950 1 s 2972 405 0 ));
+/* pglsn_ops */
+DATA(insert ( 3261 3220 3220 1 s 3222 405 0 ));
/* numeric_ops */
DATA(insert ( 1998 1700 1700 1 s 1752 405 0 ));
/* array_ops */
diff --git a/src/include/catalog/pg_amproc.h b/src/include/catalog/pg_amproc.h
index 198b126..369adb9 100644
--- a/src/include/catalog/pg_amproc.h
+++ b/src/include/catalog/pg_amproc.h
@@ -134,6 +134,7 @@ DATA(insert ( 2789 27 27 1 2794 ));
DATA(insert ( 2968 2950 2950 1 2960 ));
DATA(insert ( 2994 2249 2249 1 2987 ));
DATA(insert ( 3194 2249 2249 1 3187 ));
+DATA(insert ( 3260 3220 3220 1 3263 ));
DATA(insert ( 3522 3500 3500 1 3514
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Wed, May 7, 2014 at 8:07 AM, Andres Freund and...@2ndquadrant.com wrote:
On 2014-05-06 22:49:07 +0900, Michael Paquier wrote:
FWIW, the format you're using makes applying the patch including the
commit message relatively hard. Consider using git format-patch.
Could you be clearer? By applying a filterdiff command or by using git
diff? The latter has been used for this patch.
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+ XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+ PG_RETURN_INT32(lsn1 == lsn2);
+}
This doesn't look correct. A cmp routine needs to return -1, 0, 1 when a
b, a = b, a b respectively. You'll only return 0 and 1 here.
Thanks, I recalled that this morning as well... And my 2nd patch uses
this flow instead:
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+ XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+ if (lsn1 lsn2)
+ PG_RETURN_INT32(-1);
+ if (lsn1 lsn2)
+ PG_RETURN_INT32(1);
+ PG_RETURN_INT32(0);
+}
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn = PG_GETARG_LSN(0);
+
+ return hashint8(lsn);
+}
That can't be right either. There's at least two things wrong here:
a) hashint8 takes PG_FUNCTION_ARGS, not a Datum
In this case you may consider changing timestamp_hash@time.c and
time_hash@date.c as well :)
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 2014-05-07 08:16:38 +0900, Michael Paquier wrote:
On Wed, May 7, 2014 at 8:07 AM, Andres Freund and...@2ndquadrant.com wrote:
On 2014-05-06 22:49:07 +0900, Michael Paquier wrote:
FWIW, the format you're using makes applying the patch including the
commit message relatively hard. Consider using git format-patch.
Could you be clearer? By applying a filterdiff command or by using git
diff? The latter has been used for this patch.
As I said, consider using git format-patch. Then the patch can be
applied using git am. Resulting in a local commit including your
commit message.
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+ XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+ PG_RETURN_INT32(lsn1 == lsn2);
+}
This doesn't look correct. A cmp routine needs to return -1, 0, 1 when a
b, a = b, a b respectively. You'll only return 0 and 1 here.
Thanks, I recalled that this morning as well... And my 2nd patch uses
this flow instead:
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+ XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+ if (lsn1 lsn2)
+ PG_RETURN_INT32(-1);
+ if (lsn1 lsn2)
+ PG_RETURN_INT32(1);
+ PG_RETURN_INT32(0);
+}
That's nearly what's in the patch I attached.
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn = PG_GETARG_LSN(0);
+
+ return hashint8(lsn);
+}
That can't be right either. There's at least two things wrong here:
a) hashint8 takes PG_FUNCTION_ARGS, not a Datum
In this case you may consider changing timestamp_hash@time.c and
time_hash@date.c as well :)
Uh. They're different:
Datum
timestamp_hash(PG_FUNCTION_ARGS)
{
/* We can use either hashint8 or hashfloat8 directly */
#ifdef HAVE_INT64_TIMESTAMP
return hashint8(fcinfo);
#else
return hashfloat8(fcinfo);
#endif
}
note it's passing fcinfo, not the datum as you do. Same with
time_hash.. In fact your version crashes when used because it's
dereferencing a int8 as a pointer inside hashfloat8.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Wed, May 7, 2014 at 8:22 AM, Andres Freund and...@2ndquadrant.com wrote:
Uh. They're different:
Datum
timestamp_hash(PG_FUNCTION_ARGS)
{
/* We can use either hashint8 or hashfloat8 directly */
#ifdef HAVE_INT64_TIMESTAMP
return hashint8(fcinfo);
#else
return hashfloat8(fcinfo);
#endif
}
note it's passing fcinfo, not the datum as you do. Same with
time_hash.. In fact your version crashes when used because it's
dereferencing a int8 as a pointer inside hashfloat8.
Thanks, didn't notice that fcinfo was used.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On Tue, May 6, 2014 at 8:25 PM, Michael Paquier michael.paqu...@gmail.com
wrote:
On Wed, May 7, 2014 at 8:22 AM, Andres Freund and...@2ndquadrant.com
wrote:
Uh. They're different:
Datum
timestamp_hash(PG_FUNCTION_ARGS)
{
/* We can use either hashint8 or hashfloat8 directly */
#ifdef HAVE_INT64_TIMESTAMP
return hashint8(fcinfo);
#else
return hashfloat8(fcinfo);
#endif
}
note it's passing fcinfo, not the datum as you do. Same with
time_hash.. In fact your version crashes when used because it's
dereferencing a int8 as a pointer inside hashfloat8.
Thanks, didn't notice that fcinfo was used.
Hi all,
If helps, I added some regression tests to the lastest patch.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Timbira: http://www.timbira.com.br
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index d1448ae..c021a1e 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -14,6 +14,7 @@
#include postgres.h
#include funcapi.h
+#include access/hash.h
#include libpq/pqformat.h
#include utils/builtins.h
#include utils/pg_lsn.h
@@ -153,6 +154,27 @@ pg_lsn_ge(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(lsn1 = lsn2);
}
+/* handler for btree index operator */
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr a = PG_GETARG_LSN(0);
+ XLogRecPtr b = PG_GETARG_LSN(1);
+
+ if (a b)
+ PG_RETURN_INT32(1);
+ else if (a == b)
+ PG_RETURN_INT32(0);
+ else
+ PG_RETURN_INT32(-1);
+}
+
+/* hash index support */
+Datum
+pg_lsn_hash(PG_FUNCTION_ARGS)
+{
+ return DirectFunctionCall1(hashint8, PG_GETARG_LSN(0));
+}
/*--
* Arithmetic operators on PostgreSQL LSNs.
diff --git a/src/include/catalog/pg_amop.h b/src/include/catalog/pg_amop.h
index 8efd3be..5cd170d 100644
--- a/src/include/catalog/pg_amop.h
+++ b/src/include/catalog/pg_amop.h
@@ -513,6 +513,16 @@ DATA(insert ( 2968 2950 2950 4 s 2977 403 0 ));
DATA(insert ( 2968 2950 2950 5 s 2975 403 0 ));
/*
+ * btree pglsn_ops
+ */
+
+DATA(insert ( 3260 3220 3220 1 s 3224 403 0 ));
+DATA(insert ( 3260 3220 3220 2 s 3226 403 0 ));
+DATA(insert ( 3260 3220 3220 3 s 3222 403 0 ));
+DATA(insert ( 3260 3220 3220 4 s 3227 403 0 ));
+DATA(insert ( 3260 3220 3220 5 s 3225 403 0 ));
+
+/*
* hash index _ops
*/
@@ -581,6 +591,8 @@ DATA(insert ( 2231 1042 1042 1 s 1054 405 0 ));
DATA(insert ( 2235 1033 1033 1 s 974 405 0 ));
/* uuid_ops */
DATA(insert ( 2969 2950 2950 1 s 2972 405 0 ));
+/* pglsn_ops */
+DATA(insert ( 3261 3220 3220 1 s 3222 405 0 ));
/* numeric_ops */
DATA(insert ( 1998 1700 1700 1 s 1752 405 0 ));
/* array_ops */
diff --git a/src/include/catalog/pg_amproc.h b/src/include/catalog/pg_amproc.h
index 198b126..369adb9 100644
--- a/src/include/catalog/pg_amproc.h
+++ b/src/include/catalog/pg_amproc.h
@@ -134,6 +134,7 @@ DATA(insert ( 2789 27 27 1 2794 ));
DATA(insert ( 2968 2950 2950 1 2960 ));
DATA(insert ( 2994 2249 2249 1 2987 ));
DATA(insert ( 3194 2249 2249 1 3187 ));
+DATA(insert ( 3260 3220 3220 1 3263 ));
DATA(insert ( 3522 3500 3500 1 3514 ));
DATA(insert ( 3626 3614 3614 1 3622 ));
DATA(insert ( 3683 3615 3615 1 3668 ));
@@ -174,6 +175,7 @@ DATA(insert ( 2229 25 25 1 400 ));
DATA(insert ( 2231 1042 1042 1 1080 ));
DATA(insert ( 2235 1033 1033 1 329 ));
DATA(insert ( 2969 2950 2950 1 2963 ));
+DATA(insert ( 3261 3220 3220 1 3262 ));
DATA(insert ( 3523 3500 3500 1 3515 ));
DATA(insert ( 3903 3831 3831 1 3902 ));
DATA(insert ( 4034 3802 3802 1 4045 ));
diff --git a/src/include/catalog/pg_opclass.h b/src/include/catalog/pg_opclass.h
index ecf7063..9a45244 100644
--- a/src/include/catalog/pg_opclass.h
+++ b/src/include/catalog/pg_opclass.h
@@ -215,6 +215,8 @@ DATA(insert ( 2742 _reltime_ops PGNSP PGUID 2745 1024 t 703 ));
DATA(insert ( 2742 _tinterval_ops PGNSP PGUID 2745 1025 t 704 ));
DATA(insert ( 403 uuid_ops PGNSP PGUID 2968 2950 t 0 ));
DATA(insert ( 405 uuid_ops PGNSP PGUID 2969 2950 t 0 ));
+DATA(insert ( 403 pglsn_ops PGNSP PGUID 3260 3220 t 0 ));
+DATA(insert ( 405 pglsn_ops PGNSP PGUID 3261 3220 t 0 ));
DATA(insert ( 403 enum_ops PGNSP PGUID 3522 3500 t 0 ));
DATA(insert ( 405 enum_ops PGNSP PGUID 3523 3500 t 0 ));
DATA(insert ( 403 tsvector_ops PGNSP PGUID 3626 3614 t 0 ));
diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h
index f280af4..87ee4eb 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -1595,7 +1595,7 @@ DATA(insert OID = 2977 ( = PGNSP PGUID b f f 2950 2950 16 2976 2974 uuid_
DESCR(greater than or equal);
/* pg_lsn operators */
-DATA(insert OID = 3222 ( = PGNSP PGUID b f f 3220 3220 16 3222 3223 pg_lsn_eq eqsel
Re: [HACKERS] New pg_lsn type doesn't have hash/btree opclasses
On 05/07/2014 07:16 AM, Michael Paquier wrote: On Wed, May 7, 2014 at 8:07 AM, Andres Freund and...@2ndquadrant.com wrote: On 2014-05-06 22:49:07 +0900, Michael Paquier wrote: FWIW, the format you're using makes applying the patch including the commit message relatively hard. Consider using git format-patch. Could you be clearer? By applying a filterdiff command or by using git diff? The latter has been used for this patch. git format-patch -1 is usually what you want to emit a patch for the top commit. To produce a patchset between the current branch and master: git format-patch master It doesn't use context diff, but I think people here have mostly stopped caring about that now (?). -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
