[#7993] Update error message for insert into select from source #34872

mtabebe · 2026-01-30T18:56:38Z

https://github.com/MaterializeInc/database-issues/issues/7993

Problem:

It's not allowed for an INSERT INTO ... SELECT ... to (transitively) refer to any sources.

The reason is that with Serializability, it is not possible to take a lock on sources. And it is possible that there are writes in between the read timestamp and the write timestamp which violates serializability.

Solution:

Change the error to take two parameters: object type and object name and output this state

Testing:

Introduce new SQL logic test: insert-into-select-source-error.slt which shows the error case for inserting from a source. As well as non error cases

Motivation

This PR adds a known-desirable feature.

Tips for reviewer

Please checkout the test for positive/negative cases

Checklist

This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

ggevay

I wrote some comments.

ggevay · 2026-01-31T10:12:30Z

src/adapter/src/error.rs

+                     {object_type} '{}' may not be used in this operation; \
+                     the selection may refer to views and materialized views, but transitive \
+                     dependencies must not include sources or source-export tables",
+                    object_name.quoted()


Could you please split off the longer explanation into the AdapterError::detail? We have an error msg style guide that mentions this and various other minor things.

Also, some possible further tweaks for the message content:

I'm thinking that even the first part of the error should mention that we mean transitive dependencies, because just saying "refer" sounds like direct references, so some users might get confused.

We should mention somewhere that this is a limitation of INSERT INTO ... SELECT ... operations, and not other SELECTs.

(If the first part of the message changes, then please also update it in sqlsmith.py.)

I was able to produce the same problem with update/delete. So I don't think it makes sense to explicitly mention INSERT INTO ... SELECT ... operations here?

I think "the selection" makes sense because of the context!

ggevay · 2026-01-31T10:19:14Z

test/sqllogictest/insert-into-select-source-error.slt

+# INSERT INTO ... SELECT ... from view that transitively refers to source -> error
+statement error invalid selection
+INSERT INTO salary_exceptions (employee, salary, type)
+SELECT 'Alice', salary, type FROM emp_view LIMIT 1


You could use the simple directive to test the full error msg. For an example, see

materialize/test/sqllogictest/arrays.slt

Lines 720 to 724 in ee5d332

simple

SELECT array_cat(ARRAY[[1,2]], ARRAY[[3,4,5]]);

----

db error: ERROR: cannot concatenate incompatible arrays

DETAIL: Arrays with differing dimensions are not compatible for concatenation.

Cool, thanks!

ggevay · 2026-01-31T10:29:39Z

test/sqllogictest/insert-into-select-source-error.slt

Can we provoke this error in an UPDATE or a DELETE statement somehow? E.g. maybe with a subquery in the WHERE clause? At first glance it looks like we do allow that:

materialize/src/sql/src/plan/query.rs

Line 827 in 17d21f7

allow_subqueries: true,

ggevay · 2026-01-31T10:31:26Z

src/adapter/src/coord/sequencer/inner.rs

            };
            if !valid {
-                return Err(AdapterError::InvalidTableMutationSelection);
+                let (object_name, object_type) = match catalog.try_get_entry(id) {


Would it be maybe slightly simpler if we put the error return also into the first match above, instead of introducing the valid boolean and doing another match? I'm not sure, so if you think it's simpler with the current approach, then it's fine.

The valid boolean was already there. I kind of like keeping the error handling path separate. I had tried to write it originally with avoiding the catalog look up again. But in the end I found it a bit complicated.

I will leave it as is, unless others feel strongly.

SangJunBak

Agree to all of Gabor's feedback, otw code looks good to me~

One thing I noticed is if you have multiple transitive source dependencies, we only call out the first one when it'd be nice to call out all of them. I don't think this needs to be blocking, but if you're up for it, we can create a followup github issue/PR! Or even add onto this PR. Totally up to you though

ggevay · 2026-02-02T16:00:17Z

src/adapter/src/error.rs

+            AdapterError::InvalidTableMutationSelection { .. } => {
+                write!(
+                    f,
+                    "invalid selection: operation may only refer to user-defined tables, even transitively."


Some minor things:

CREATE TABLE FROM SOURCE is also kind of a user-defined table, but is not allowed here. How about instead of "user-defined tables", saying "non-source, non-system tables"?

I wouldn't say "even" here, because that sounds like as if all dependencies could only be tables, but actually views and MVs can also be among the (intermediate) dependencies.

The error msg style guide says no punctuation at the end of the primary message.

So, how about something like:

invalid selection: operation may only (transitively) refer to non-source, non-system tables

mtabebe · 2026-02-02T16:28:06Z

Agree to all of Gabor's feedback, otw code looks good to me~

One thing I noticed is if you have multiple transitive source dependencies, we only call out the first one when it'd be nice to call out all of them. I don't think this needs to be blocking, but if you're up for it, we can create a followup github issue/PR! Or even add onto this PR. Totally up to you though

I will create a follow up issue. In general, what is the philosophy on error messages? Do you think it is better to try and report as many errors as possible in one? Or just have a single error that a user can then iteratively address?

Problem: It's not allowed for an `INSERT INTO ... SELECT ...` to (transitively) refer to any sources. The reason is that with Serializability, it is not possible to take a lock on sources. And it is possible that there are writes in between the read timestamp and the write timestamp which violates serializability. Solution: Change the error to take two parameters: object type and object name and output this state Testing: Introduce new SQL logic test: insert-into-select-source-error.slt which shows the error case for inserting from a source. As well as non error cases Additionally show the error arises for update and delete

ggevay · 2026-02-02T16:51:18Z

We often just stop at the first error, but I think Jun has a good point that in this case it would be useful to report as many as possible, so that the user immediately has an overview of all the issues. But I also wouldn't block this PR on this.

ggevay

Thank you, looks good to me!

mtabebe requested a review from a team as a code owner January 30, 2026 18:56

mtabebe requested review from SangJunBak, aljoscha and ggevay January 30, 2026 18:56

ggevay reviewed Jan 31, 2026

View reviewed changes

mtabebe force-pushed the ma/7993/error-msg-insert-into-select branch from 81f53e9 to 047faa1 Compare February 2, 2026 15:24

mtabebe requested a review from ggevay February 2, 2026 15:24

SangJunBak reviewed Feb 2, 2026

View reviewed changes

ggevay reviewed Feb 2, 2026

View reviewed changes

mtabebe force-pushed the ma/7993/error-msg-insert-into-select branch from 047faa1 to fe6fe4e Compare February 2, 2026 16:45

mtabebe requested a review from a team as a code owner February 2, 2026 16:45

mtabebe requested review from SangJunBak and ggevay February 2, 2026 16:45

ggevay approved these changes Feb 2, 2026

View reviewed changes

mtabebe merged commit 7e81492 into MaterializeInc:main Feb 2, 2026
132 checks passed

	simple
	SELECT array_cat(ARRAY[[1,2]], ARRAY[[3,4,5]]);
	----
	db error: ERROR: cannot concatenate incompatible arrays
	DETAIL: Arrays with differing dimensions are not compatible for concatenation.

[#7993] Update error message for insert into select from source #34872

[#7993] Update error message for insert into select from source #34872

Conversation

mtabebe commented Jan 30, 2026

Problem:

Solution:

Testing:

Motivation

Tips for reviewer

Checklist

Uh oh!

ggevay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggevay Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SangJunBak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtabebe commented Feb 2, 2026

Uh oh!

ggevay commented Feb 2, 2026

Uh oh!

ggevay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ggevay Jan 31, 2026 •

edited

Loading

SangJunBak left a comment •

edited

Loading