Noah,
* Noah Misch (n...@leadboat.com) wrote:
On Mon, Feb 14, 2011 at 04:06:59PM -0500, Stephen Frost wrote:
I've attached a new version of the patch that attempts to flesh out the
comments
based on your feedback. Does it improve things?
Yes, much better, thanks!
Offhand, I can't think
On Sun, Feb 13, 2011 at 02:53:07PM -0500, Robert Haas wrote:
On Sun, Feb 13, 2011 at 6:50 AM, Noah Misch n...@leadboat.com wrote:
Yikes.
?I didn't like that Assert much, but maybe we need it, because this is
scary.
Can you elaborate on the fear-inducing aspect? ?I don't mind re-adding
Noah,
I'm even less familiar w/ this code than Robert, but figured I'd give a
shot at reviewing this anyway. I definitely like avoiding table
rewrites if I can get away with it. :)
First question is- why do you use #ifdef USE_ASSERT_CHECKING ..?
assert_enabled exists and will work the way you
Hi Stephen,
Thanks for jumping in on this one.
On Mon, Feb 14, 2011 at 01:12:21PM -0500, Stephen Frost wrote:
First question is- why do you use #ifdef USE_ASSERT_CHECKING ..?
The other six code sites checking assert_enabled directly do the same.
assert_enabled exists and will work the way
* Noah Misch (n...@leadboat.com) wrote:
On Mon, Feb 14, 2011 at 01:12:21PM -0500, Stephen Frost wrote:
First question is- why do you use #ifdef USE_ASSERT_CHECKING ..?
The other six code sites checking assert_enabled directly do the same.
Wow, I could have sworn that I looked at what others
On Mon, Feb 14, 2011 at 04:06:59PM -0500, Stephen Frost wrote:
* Noah Misch (n...@leadboat.com) wrote:
On Mon, Feb 14, 2011 at 01:12:21PM -0500, Stephen Frost wrote:
In ATColumnChangeSetWorkLevel(), I'm really not a huge fan of using a
passed-in argument to move through a list with.. I'd
On Mon, Feb 14, 2011 at 7:52 AM, Noah Misch n...@leadboat.com wrote:
I'm half-tempted to put that part off to
9.2, in the hopes of getting a more substantial solution that can also
handle things like text - xml which we don't have time to re-engineer
right now.
I see.
After sleeping on it,
On Sun, Feb 13, 2011 at 12:04:20AM -0500, Robert Haas wrote:
On Sat, Feb 12, 2011 at 10:45 AM, Noah Misch n...@leadboat.com wrote:
That said, I've tried both constructions, and I marginally prefer the end
result
with AlteredTableInfo.verify. ?I've inlined ATColumnChangeRequiresRewrite
On Sun, Feb 13, 2011 at 6:50 AM, Noah Misch n...@leadboat.com wrote:
Yikes.
I didn't like that Assert much, but maybe we need it, because this is
scary.
Can you elaborate on the fear-inducing aspect? I don't mind re-adding the
Assert, but it seems that some positive understanding of the
On Fri, Feb 11, 2011 at 02:49:27PM -0500, Robert Haas wrote:
You might want to consider a second boolean in lieu of a three way
enum. I'm not sure if that's cleaner but if it lets you write:
if (blah)
at-verify = true;
instead of:
if (blah)
at-worklevel = Min(at-worklevel,
On Sat, Feb 12, 2011 at 10:45 AM, Noah Misch n...@leadboat.com wrote:
That said, I've tried both constructions, and I marginally prefer the end
result
with AlteredTableInfo.verify. I've inlined ATColumnChangeRequiresRewrite into
ATPrepAlterColumnType; it would need to either pass back two
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
On Wed, Jan 26, 2011 at 07:31:40AM -0500, Robert Haas wrote:
I'd also suggest that this big if-block you changed to a case
statement could just as well stay as an if-block. There are only
three cases, and we want to avoid
Hi Robert,
On Fri, Feb 11, 2011 at 10:27:11AM -0500, Robert Haas wrote:
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
Done as attached. ?This preserves compatibility with our current handling of
composite type dependencies. ?The rest you've seen before.
OK, I
On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch n...@leadboat.com wrote:
It's a nice, clean patch. However, it achieves that by targeting a smaller
goal. Specifically, it omits:
1) Test cases and DEBUG messages sufficient to facilitate them
That was an intentional omission.
2) Skip rewrite
On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch n...@leadboat.com wrote:
Even supposing we push off all scan-only cases to another patch, it would be
good to have the tablecmds.c-internal representation of that in mind. No
sense
in simplifying a 12-line change to an 8-line change, only to redo
On Fri, Feb 11, 2011 at 01:55:45PM -0500, Robert Haas wrote:
On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch n...@leadboat.com wrote:
Even supposing we push off all scan-only cases to another patch, it would be
good to have the tablecmds.c-internal representation of that in mind. ?No
sense
On Fri, Feb 11, 2011 at 2:17 PM, Noah Misch n...@leadboat.com wrote:
Good to know. I can envision that perspective, and I share it when the
savings
is rather more substantial, say 10% of the patch or 100 lines. Below that
threshold, the energy I expend grasping two interface changes in one
On Sun, Feb 06, 2011 at 02:15:47AM -0500, Robert Haas wrote:
On Sat, Feb 5, 2011 at 7:44 PM, Noah Misch n...@leadboat.com wrote:
But this is a little unsatisfying, for two reasons. ?First, the error
message will be subtly wrong: we can make it complain about a table or
a type, but not a
On Sun, Feb 6, 2011 at 4:15 AM, Noah Misch n...@leadboat.com wrote:
That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy. So I made it take a relkind and a
name, which works fine.
Hmm, indeed. In get_rels_with_domain(), it's a scalar type.
On Sun, Feb 06, 2011 at 07:54:52AM -0500, Robert Haas wrote:
On Sun, Feb 6, 2011 at 4:15 AM, Noah Misch n...@leadboat.com wrote:
That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy. ?So I made it take a relkind and a
name, which works fine.
On Sun, Feb 06, 2011 at 08:40:44AM -0500, Noah Misch wrote:
On Sun, Feb 06, 2011 at 07:54:52AM -0500, Robert Haas wrote:
Yeeah, that's actually a little ugly. It's actually a domain
over a composite type, not a composite type proper, IIUC. Better
ideas?
There are no domains over
On Sun, Feb 6, 2011 at 8:58 AM, Noah Misch n...@leadboat.com wrote:
On Sun, Feb 06, 2011 at 08:40:44AM -0500, Noah Misch wrote:
On Sun, Feb 06, 2011 at 07:54:52AM -0500, Robert Haas wrote:
Yeeah, that's actually a little ugly. It's actually a domain
over a composite type, not a
On Sun, Feb 6, 2011 at 12:13 PM, Robert Haas robertmh...@gmail.com wrote:
That's not quite so good for translators, I think.
Another option is that we could just say relation (table, foreign
table, etc...) or type. We use the word relation as a more generic
version of table in a few other
On Sun, Feb 06, 2011 at 12:54:19PM -0500, Robert Haas wrote:
On Sun, Feb 6, 2011 at 12:13 PM, Robert Haas robertmh...@gmail.com wrote:
That's not quite so good for translators, I think.
Another option is that we could just say relation (table, foreign
table, etc...) or type. ?We use the
On Sun, Feb 6, 2011 at 8:18 PM, Noah Misch n...@leadboat.com wrote:
Or how about passing an ObjectType? Then we could specify
OBJECT_TABLE, OBJECT_FOREIGN_TABLE, or OBJECT_TYPE.
Could this be done without a several-line blob of code at each call site to
determine the answer? If and only if
On Mon, Feb 07, 2011 at 12:04:02AM -0500, Robert Haas wrote:
On Sun, Feb 6, 2011 at 8:18 PM, Noah Misch n...@leadboat.com wrote:
Or how about passing an ObjectType? ?Then we could specify
OBJECT_TABLE, OBJECT_FOREIGN_TABLE, or OBJECT_TYPE.
Could this be done without a several-line blob of
Robert,
Thanks for the obviously thought-out review.
On Sat, Feb 05, 2011 at 01:29:35AM -0500, Robert Haas wrote:
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
Done as attached. ?This preserves compatibility with our current handling of
composite type dependencies.
On Sat, Feb 5, 2011 at 3:05 AM, Noah Misch n...@leadboat.com wrote:
Correct. It's perhaps obvious, but rewriting the table will always reindex
it.
Right.
3. If we're changing the data type of a column in the table, reindex the
table.
Rebuild indexes that depend on a changing column.
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
Done as attached.
Looking at this still more, it appears that independent of any change
this patch may wish to make, there's a live bug here related to the
foreign table patch I committed back in December. Creating a foreign
On Sat, Feb 05, 2011 at 10:03:59AM -0500, Robert Haas wrote:
Looking at this still more, it appears that independent of any change
this patch may wish to make, there's a live bug here related to the
foreign table patch I committed back in December. Creating a foreign
table creates an
On Sat, Feb 5, 2011 at 7:44 PM, Noah Misch n...@leadboat.com wrote:
But this is a little unsatisfying, for two reasons. First, the error
message will be subtly wrong: we can make it complain about a table or
a type, but not a foreign table. At a quick look, it likes the right
fix might be to
On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch n...@leadboat.com wrote:
Done as attached. This preserves compatibility with our current handling of
composite type dependencies. The rest you've seen before.
OK, so I took a look at this in more detail today. The current logic
for table rewrites
On Wed, Jan 26, 2011 at 07:31:40AM -0500, Robert Haas wrote:
I'd also suggest that this big if-block you changed to a case
statement could just as well stay as an if-block. There are only
three cases, and we want to avoid rearranging things more than
necessary. It complicates both review and
On Tue, Jan 25, 2011 at 10:22 PM, Noah Misch n...@leadboat.com wrote:
I'm fine with this patch.
OK, committed.
The next patch removed new_changeoids, so we would instead be keeping it with
this as the only place referencing it.
[...]
The at2v2 patch would then morph to do something like:
On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch n...@leadboat.com wrote:
* at1.1-default-composite.patch
Remove the error when the user adds a column with a default value to a table
whose rowtype is used in a column elsewhere.
Can we fix this without moving the logic around quite so much? I'm
On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch n...@leadboat.com wrote:
* at1.2-doc-set-data-type.patch
The documentation used ALTER TYPE when it meant SET DATA TYPE, a subform
of
ALTER TABLE or ALTER FOREIGN TABLE. Fixes just that.
Committed this part. For reasons involving me being tired, I
On Tue, Jan 25, 2011 at 06:40:08PM -0500, Robert Haas wrote:
On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch n...@leadboat.com wrote:
* at1.1-default-composite.patch
Remove the error when the user adds a column with a default value to a table
whose rowtype is used in a column elsewhere.
Can
Robert Haas robertmh...@gmail.com writes:
On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch n...@leadboat.com wrote:
As unintended fallout, it's no longer an error to add oids or a column with a
default value to a table whose rowtype is used in columns elsewhere. This
seems
best. Defaults on the
On Sun, Jan 23, 2011 at 12:06 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Robert Haas robertmh...@gmail.com writes:
On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch n...@leadboat.com wrote:
As unintended fallout, it's no longer an error to add oids or a column with
a
default value to a table whose
On Sun, Jan 23, 2011 at 12:06:43PM -0500, Tom Lane wrote:
Robert Haas robertmh...@gmail.com writes:
On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch n...@leadboat.com wrote:
As unintended fallout, it's no longer an error to add oids or a column
with a
default value to a table whose rowtype is
On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch n...@leadboat.com wrote:
This patch removes ALTER TYPE rewrites in cases we can already prove valid. I
add a function GetCoerceExemptions() that walks an Expr according to rules
discussed in the design thread, simplified slightly pending additions in
This patch removes ALTER TYPE rewrites in cases we can already prove valid. I
add a function GetCoerceExemptions() that walks an Expr according to rules
discussed in the design thread, simplified slightly pending additions in the
next patch. See the comment at that function for a refresher. I
42 matches
Mail list logo
