Re: [HACKERS] Second thoughts on CheckIndexCompatible() vs. operator families
Noah Misch writes: > On Thu, Jan 26, 2012 at 08:23:34AM -0500, Robert Haas wrote: >> I'm just going to remove the test. This is not very future-proof and > [ objections ] FWIW, I concur with Robert's choice here. This test method is ugly and fragile, and I'm not even thinking about the question of whether an installation might have GUC settings that affect it. My objection is that you're assuming that nowhere else, anywhere in the large amount of code executed by the queries under test, will anyone ever wish to insert another elog(DEBUG) message. > I used the same strategy in another ALTER TABLE patch this CF: > http://archives.postgresql.org/message-id/20120126033956.gc15...@tornado.leadboat.com That's going to need to be removed before commit too, then. 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] Second thoughts on CheckIndexCompatible() vs. operator families
On Thu, Jan 26, 2012 at 08:23:34AM -0500, Robert Haas wrote: > On Thu, Jan 26, 2012 at 6:55 AM, Noah Misch wrote: > > On Wed, Jan 25, 2012 at 11:53:10PM -0500, Tom Lane wrote: > >> Not only is that code spectacularly unreadable, but has nobody noticed > >> that this commit broke the buildfarm? > > > > Thanks for reporting the problem. ?This arose because the new test case > > temporarily sets client_min_messages=DEBUG1. ?The default buildfarm > > configuration sets log_statement=all in its postgresql.conf, so the client > > gets those log_statement lines. ?I would fix this as attached, resetting the > > optional logging to defaults during the test cases in question. ?Not > > delightful, but that's what we have to work with. > > I'm just going to remove the test. This is not very future-proof and It would deserve an update whenever we add a new optional-logging GUC pertaining to user backends. Other tests require similarly-infrequent refreshes in response to other changes. Of course, buildfarm members would not be setting those new GUCs the day they're available. Calling for an update to the test could reasonably fall to the first buildfarm member owner who actually decides to use a new GUC in his configuration. > an ugly pattern if it gets copied to other places. We need to work on I would rather folks introduce ugliness to the test suite than introduce behaviors having no test coverage. > a more sensible way for ALTER TABLE to report what it did, but a > solution based on what GUCs the build-farm happens to set doesn't seem > like it's justified for the narrowness of the case we're testing here. It's not based on what GUCs the buildfarm happens to set. I looked up all GUCs that might create problems such as the one observed here. They were the four I included in the patch, plus debug_pretty_print, debug_print_parse, debug_print_plan and debug_print_rewritten. I concluded that the four debug_* ones were materially less likely to ever get set in postgresql.conf for a "make installcheck" run, so I left them out for brevity. The setting changed *by default* for buildfarm clients is log_statement. Buildfarm member owners can do as they wish, though. > Whether or not we allow this case to work without a rewrite is in > some sense arbitrary. There's no real reason it can't be done; rather, > we're just exercising restraint to minimize the risk of future bugs. > So I don't want to go to great lengths to test it. I used the same strategy in another ALTER TABLE patch this CF: http://archives.postgresql.org/message-id/20120126033956.gc15...@tornado.leadboat.com If we pay its costs for those tests, we then may as well let this test case ride their coattails. -- 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] Second thoughts on CheckIndexCompatible() vs. operator families
On Thu, Jan 26, 2012 at 6:55 AM, Noah Misch wrote: > On Wed, Jan 25, 2012 at 11:53:10PM -0500, Tom Lane wrote: >> Not only is that code spectacularly unreadable, but has nobody noticed >> that this commit broke the buildfarm? > > Thanks for reporting the problem. This arose because the new test case > temporarily sets client_min_messages=DEBUG1. The default buildfarm > configuration sets log_statement=all in its postgresql.conf, so the client > gets those log_statement lines. I would fix this as attached, resetting the > optional logging to defaults during the test cases in question. Not > delightful, but that's what we have to work with. I'm just going to remove the test. This is not very future-proof and an ugly pattern if it gets copied to other places. We need to work on a more sensible way for ALTER TABLE to report what it did, but a solution based on what GUCs the build-farm happens to set doesn't seem like it's justified for the narrowness of the case we're testing here. Whether or not we allow this case to work without a rewrite is in some sense arbitrary. There's no real reason it can't be done; rather, we're just exercising restraint to minimize the risk of future bugs. So I don't want to go to great lengths to test it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Second thoughts on CheckIndexCompatible() vs. operator families
On Wed, Jan 25, 2012 at 11:53:10PM -0500, Tom Lane wrote: > Not only is that code spectacularly unreadable, but has nobody noticed > that this commit broke the buildfarm? Thanks for reporting the problem. This arose because the new test case temporarily sets client_min_messages=DEBUG1. The default buildfarm configuration sets log_statement=all in its postgresql.conf, so the client gets those log_statement lines. I would fix this as attached, resetting the optional logging to defaults during the test cases in question. Not delightful, but that's what we have to work with. nm *** a/src/test/regress/expected/alter_table.out --- b/src/test/regress/expected/alter_table.out *** *** 1665,1671 where oid = 'test_storage'::regclass; --- 1665,1679 t (1 row) + -- -- SET DATA TYPE without a rewrite + -- + -- Temporarily disable optional logging that "make installcheck" testers + -- reasonably might enable. + SET log_duration = off; + SET log_lock_waits = off; + SET log_statement = none; + SET log_temp_files = -1; CREATE DOMAIN other_textarr AS text[]; CREATE TABLE norewrite_array(c text[] PRIMARY KEY); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "norewrite_array_pkey" for table "norewrite_array" *** *** 1674,1679 ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work --- 1682,1691 ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index DEBUG: building index "norewrite_array_pkey" on table "norewrite_array" RESET client_min_messages; + RESET log_duration; + RESET log_lock_waits; + RESET log_statement; + RESET log_temp_files; -- -- lock levels -- *** a/src/test/regress/sql/alter_table.sql --- b/src/test/regress/sql/alter_table.sql *** *** 1197,1203 select reltoastrelid <> 0 as has_toast_table --- 1197,1213 from pg_class where oid = 'test_storage'::regclass; + -- -- SET DATA TYPE without a rewrite + -- + + -- Temporarily disable optional logging that "make installcheck" testers + -- reasonably might enable. + SET log_duration = off; + SET log_lock_waits = off; + SET log_statement = none; + SET log_temp_files = -1; + CREATE DOMAIN other_textarr AS text[]; CREATE TABLE norewrite_array(c text[] PRIMARY KEY); SET client_min_messages = debug1; *** *** 1205,1210 ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work --- 1215,1225 ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index RESET client_min_messages; + RESET log_duration; + RESET log_lock_waits; + RESET log_statement; + RESET log_temp_files; + -- -- lock levels -- -- 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] Second thoughts on CheckIndexCompatible() vs. operator families
Alvaro Herrera writes:
> Excerpts from Robert Haas's message of mié ene 25 17:32:49 -0300 2012:
>> On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch wrote:
> New version that repairs a defective test case.
>>
>> Committed. I don't find this to be particularly good style:
>>
>> + for (i = 0; i < old_natts && ret; i++)
>> + ret =
>> (!IsPolymorphicType(get_opclass_input_type(classObjectId[i
>> + irel->rd_att->attrs[i]->atttypid ==
>> typeObjectId[i]);
>>
>> ...but I am not sure whether we have any formal policy against it, so
>> I just committed it as-is for now. I would have surrounded the loop
>> with an if (ret) block and written the body of the loop as if
>> (condition) { ret = false; break; }.
> I find that code way too clever.
Not only is that code spectacularly unreadable, but has nobody noticed
that this commit broke the buildfarm?
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] Second thoughts on CheckIndexCompatible() vs. operator families
On Wed, Jan 25, 2012 at 03:32:49PM -0500, Robert Haas wrote:
> On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch wrote:
> > New version that repairs a defective test case.
>
> Committed. I don't find this to be particularly good style:
Thanks.
> + for (i = 0; i < old_natts && ret; i++)
> + ret =
> (!IsPolymorphicType(get_opclass_input_type(classObjectId[i
> + irel->rd_att->attrs[i]->atttypid ==
> typeObjectId[i]);
>
> ...but I am not sure whether we have any formal policy against it, so
> I just committed it as-is for now. I would have surrounded the loop
> with an if (ret) block and written the body of the loop as if
> (condition) { ret = false; break; }.
I value the savings in vertical space more than the lost idiomaticness. This
decision is 90+% subjective, so I cannot blame you for concluding otherwise.
I do know the feeling of looking at PostgreSQL source code and wishing the
author had not attempted to conserve every line.
--
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] Second thoughts on CheckIndexCompatible() vs. operator families
Excerpts from Robert Haas's message of mié ene 25 19:05:44 -0300 2012:
>
> On Wed, Jan 25, 2012 at 3:52 PM, Alvaro Herrera
> wrote:
> >
> > Excerpts from Robert Haas's message of mié ene 25 17:32:49 -0300 2012:
> >> On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch wrote:
> >> > New version that repairs a defective test case.
> >>
> >> Committed. I don't find this to be particularly good style:
> >>
> >> + for (i = 0; i < old_natts && ret; i++)
> >> + ret =
> >> (!IsPolymorphicType(get_opclass_input_type(classObjectId[i
> >> + irel->rd_att->attrs[i]->atttypid ==
> >> typeObjectId[i]);
> >>
> >> ...but I am not sure whether we have any formal policy against it, so
> >> I just committed it as-is for now. I would have surrounded the loop
> >> with an if (ret) block and written the body of the loop as if
> >> (condition) { ret = false; break; }.
> >
> > I find that code way too clever.
>
> The way he wrote it, or the way I proposed to write it?
The code as committed.
--
Álvaro Herrera
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
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] Second thoughts on CheckIndexCompatible() vs. operator families
On Wed, Jan 25, 2012 at 3:52 PM, Alvaro Herrera
wrote:
>
> Excerpts from Robert Haas's message of mié ene 25 17:32:49 -0300 2012:
>> On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch wrote:
>> > New version that repairs a defective test case.
>>
>> Committed. I don't find this to be particularly good style:
>>
>> + for (i = 0; i < old_natts && ret; i++)
>> + ret =
>> (!IsPolymorphicType(get_opclass_input_type(classObjectId[i
>> + irel->rd_att->attrs[i]->atttypid ==
>> typeObjectId[i]);
>>
>> ...but I am not sure whether we have any formal policy against it, so
>> I just committed it as-is for now. I would have surrounded the loop
>> with an if (ret) block and written the body of the loop as if
>> (condition) { ret = false; break; }.
>
> I find that code way too clever.
The way he wrote it, or the way I proposed to write it?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
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] Second thoughts on CheckIndexCompatible() vs. operator families
Excerpts from Robert Haas's message of mié ene 25 17:32:49 -0300 2012:
> On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch wrote:
> > New version that repairs a defective test case.
>
> Committed. I don't find this to be particularly good style:
>
> + for (i = 0; i < old_natts && ret; i++)
> + ret =
> (!IsPolymorphicType(get_opclass_input_type(classObjectId[i
> + irel->rd_att->attrs[i]->atttypid ==
> typeObjectId[i]);
>
> ...but I am not sure whether we have any formal policy against it, so
> I just committed it as-is for now. I would have surrounded the loop
> with an if (ret) block and written the body of the loop as if
> (condition) { ret = false; break; }.
I find that code way too clever.
--
Álvaro Herrera
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
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] Second thoughts on CheckIndexCompatible() vs. operator families
On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch wrote:
> New version that repairs a defective test case.
Committed. I don't find this to be particularly good style:
+ for (i = 0; i < old_natts && ret; i++)
+ ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i
+ irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);
...but I am not sure whether we have any formal policy against it, so
I just committed it as-is for now. I would have surrounded the loop
with an if (ret) block and written the body of the loop as if
(condition) { ret = false; break; }.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
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] Second thoughts on CheckIndexCompatible() vs. operator families
New version that repairs a defective test case.
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 712b0b0..1bf1de5 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***
*** 23,28
--- 23,29
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
+ #include "catalog/pg_type.h"
#include "commands/dbcommands.h"
#include "commands/defrem.h"
#include "commands/tablecmds.h"
***
*** 52,57
--- 53,59
/* non-export function prototypes */
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
+ Oid *typeOidP,
Oid *collationOidP,
Oid *classOidP,
int16 *colOptionP,
***
*** 87,104 static void RangeVarCallbackForReindexIndex(const RangeVar
*relation,
* of columns and that if one has an expression column or predicate, both do.
* Errors arising from the attribute list still apply.
*
! * Most column type changes that can skip a table rewrite will not invalidate
! * indexes. For btree and hash indexes, we assume continued validity when
! * each column of an index would have the same operator family before and
! * after the change. Since we do not document a contract for GIN or GiST
! * operator families, we require an exact operator class match for them and
! * for any other access methods.
! *
! * DefineIndex always verifies that each exclusion operator shares an operator
! * family with its corresponding index operator class. For access methods
! * having no operator family contract, confirm that the old and new indexes
! * use the exact same exclusion operator. For btree and hash, there's nothing
! * more to check.
*
* We do not yet implement a test to verify compatibility of expression
* columns or predicates, so assume any such index is incompatible.
--- 89,105
* of columns and that if one has an expression column or predicate, both do.
* Errors arising from the attribute list still apply.
*
! * Most column type changes that can skip a table rewrite do not invalidate
! * indexes. We ackowledge this when all operator classes, collations and
! * exclusion operators match. Though we could further permit intra-opfamily
! * changes for btree and hash indexes, that adds subtle complexity with no
! * concrete benefit for core types.
!
! * When a comparison or exclusion operator has a polymorphic input type, the
! * actual input types must also match. This defends against the possibility
! * that operators could vary behavior in response to get_fn_expr_argtype().
! * At present, this hazard is theoretical: check_exclusion_constraint() and
! * all core index access methods decline to set fn_expr for such calls.
*
* We do not yet implement a test to verify compatibility of expression
* columns or predicates, so assume any such index is incompatible.
***
*** 111,116 CheckIndexCompatible(Oid oldId,
--- 112,118
List *exclusionOpNames)
{
boolisconstraint;
+ Oid*typeObjectId;
Oid*collationObjectId;
Oid*classObjectId;
Oid accessMethodId;
***
*** 123,132 CheckIndexCompatible(Oid oldId,
int numberOfAttributes;
int old_natts;
boolisnull;
- boolfamily_am;
boolret = true;
oidvector *old_indclass;
oidvector *old_indcollation;
int i;
Datum d;
--- 125,134
int numberOfAttributes;
int old_natts;
boolisnull;
boolret = true;
oidvector *old_indclass;
oidvector *old_indcollation;
+ Relationirel;
int i;
Datum d;
***
*** 168,177 CheckIndexCompatible(Oid oldId,
indexInfo->ii_ExclusionOps = NULL;
indexInfo->ii_ExclusionProcs = NULL;
indexInfo->ii_ExclusionStrats = NULL;
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
--- 170,181
indexInfo->ii_ExclusionOps =
