Skip to content

Comments

fix: generated column on public schema#201

Merged
tianzhou merged 1 commit intomainfrom
public_schema_generarated_column
Dec 17, 2025
Merged

fix: generated column on public schema#201
tianzhou merged 1 commit intomainfrom
public_schema_generarated_column

Conversation

@tianzhou
Copy link
Contributor

Fix #194

Copilot AI review requested due to automatic review settings December 17, 2025 15:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes issue #194 related to generated columns that reference functions from different schemas. The fix ensures that cross-schema function references in generated column expressions are properly qualified with their schema names.

Key Changes:

  • Modified the GetColumnsForSchema SQL query to use a LATERAL join with set_config to control the search_path when evaluating generated column expressions
  • Added test data demonstrating a generated column that calls a function from a different schema (util.extract_domain)
  • Improved parameter naming from dollar_1 to tableSchema in the Go code

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
ir/queries/queries.sql Refactored GetColumnsForSchema query with CTE and LATERAL join to properly qualify cross-schema function references in generated columns
ir/queries/queries.sql.go Generated Go code reflecting the SQL query changes and improved parameter naming
testdata/dump/tenant/util.sql Added extract_domain function to test cross-schema function usage
testdata/dump/tenant/tenant.sql Added website column and domain generated column to users table for testing
testdata/dump/tenant/pgschema.sql Expected output showing properly qualified function reference in generated column

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ge.generated_expr
FROM column_base cb
LEFT JOIN LATERAL (
SELECT
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dummy column in the LATERAL subquery is used solely to ensure set_config is evaluated before pg_get_expr. While this works, it's a workaround that relies on left-to-right evaluation order in the SELECT clause. Consider adding a comment explaining this pattern or using a more explicit approach, such as a custom function that wraps pg_get_expr with search_path control.

Suggested change
SELECT
SELECT
-- The dummy column ensures set_config is evaluated (for its side effect)
-- before pg_get_expr, relying on PostgreSQL's left-to-right SELECT evaluation.

Copilot uses AI. Check for mistakes.
FROM column_base cb
LEFT JOIN LATERAL (
SELECT
set_config('search_path', cb.table_schema || ', pg_catalog', true) as dummy,
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The search_path is constructed using string concatenation: cb.table_schema || ', pg_catalog'. While schema names from information_schema.columns are generally safe, PostgreSQL allows schema names with special characters, quotes, or commas. If a schema name contains a comma or other special characters, this could lead to unexpected behavior or potentially a security issue. Consider using proper quoting for the schema name, such as using quote_ident() or validating that the schema name doesn't contain problematic characters.

Suggested change
set_config('search_path', cb.table_schema || ', pg_catalog', true) as dummy,
set_config('search_path', format('%I, pg_catalog', cb.table_schema), true) as dummy,

Copilot uses AI. Check for mistakes.
FROM column_base cb
LEFT JOIN LATERAL (
SELECT
set_config('search_path', cb.table_schema || ', pg_catalog', true) as dummy,
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The search_path is constructed using string concatenation: cb.table_schema || ', pg_catalog'. While schema names from information_schema.columns are generally safe, PostgreSQL allows schema names with special characters, quotes, or commas. If a schema name contains a comma or other special characters, this could lead to unexpected behavior or potentially a security issue. Consider using proper quoting for the schema name, such as using quote_ident() or validating that the schema name doesn't contain problematic characters.

Suggested change
set_config('search_path', cb.table_schema || ', pg_catalog', true) as dummy,
set_config('search_path', quote_ident(cb.table_schema) || ', pg_catalog', true) as dummy,

Copilot uses AI. Check for mistakes.
Comment on lines +468 to +469
func (q *Queries) GetColumnsForSchema(ctx context.Context, tableSchema sql.NullString) ([]GetColumnsForSchemaRow, error) {
rows, err := q.db.QueryContext(ctx, getColumnsForSchema, tableSchema)
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter name tableSchema is more descriptive than dollar_1, but this creates inconsistency with other similar functions in this file (e.g., GetAggregatesForSchema, GetConstraintsForSchema, etc.) which still use dollar_1. Consider updating all similar functions to use descriptive parameter names for consistency.

Suggested change
func (q *Queries) GetColumnsForSchema(ctx context.Context, tableSchema sql.NullString) ([]GetColumnsForSchemaRow, error) {
rows, err := q.db.QueryContext(ctx, getColumnsForSchema, tableSchema)
func (q *Queries) GetColumnsForSchema(ctx context.Context, dollar_1 sql.NullString) ([]GetColumnsForSchemaRow, error) {
rows, err := q.db.QueryContext(ctx, getColumnsForSchema, dollar_1)

Copilot uses AI. Check for mistakes.
@tianzhou tianzhou force-pushed the public_schema_generarated_column branch from 661747c to 0ebbde8 Compare December 17, 2025 16:01
@tianzhou tianzhou merged commit fde44cc into main Dec 17, 2025
2 checks passed
@tianzhou tianzhou deleted the public_schema_generarated_column branch December 18, 2025 07:24
alecthomas pushed a commit to alecthomas/pgschema that referenced this pull request Jan 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

references to functions in generated column definitions should be fully qualified

1 participant