Skip to content

Conversation

JavierJF
Copy link
Collaborator

  • Reduce 'check_type' range to uniquely supported check ('read_only').
  • Added table upgrade procedure.
  • Fixed missing space in table definition (display for '\G').

- Reduce 'check_type' range to uniquely supported check ('read_only').
- Added table upgrade procedure.
- Fixed missing space in table definition (display for '\G').
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @JavierJF, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on refining the schema and ensuring robust upgrades for the pgsql_replication_hostgroups table. It narrows the allowed values for the check_type column to only the currently supported option and introduces an automated, online upgrade mechanism to seamlessly migrate existing installations to the new schema definition.

Highlights

  • Refined check_type Constraint: The check_type column in the pgsql_replication_hostgroups table (both in configuration and runtime definitions) has been updated to strictly enforce 'read_only' as the only allowed value. This removes previously supported, but apparently unused or unsupported, options like innodb_read_only or combined checks.
  • Automated Schema Upgrade Procedure: A new disk upgrade procedure has been implemented for the pgsql_replication_hostgroups table. This procedure detects if the table is using an older schema version (pre-3.0.2), renames the existing table, creates a new one with the updated definition, and then migrates the data, ensuring the check_type column is correctly set to 'read_only' during the migration.
  • Integration into Bootstrap Process: The newly added schema upgrade logic for pgsql_replication_hostgroups is now integrated into the ProxySQL_Admin initialization process, ensuring that the table is automatically upgraded when the system starts up if an older version is detected.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The code changes reduce the 'check_type' range for 'pgsql_replication_hostgroups' to uniquely supported check ('read_only'), add a table upgrade procedure, and fix a missing space in the table definition.

Comment on lines +612 to +642
void ProxySQL_Admin::disk_upgrade_pgsql_replication_hostgroups() {
configdb->execute("PRAGMA foreign_keys = OFF");

int rci = configdb->check_table_structure(
const_cast<char*>("pgsql_replication_hostgroups"),
const_cast<char*>(ADMIN_SQLITE_TABLE_PGSQL_REPLICATION_HOSTGROUPS_V3_0_1)
);

if (rci) {
// upgrade is required
proxy_warning("Detected version pre-3.0.2 of table 'pgsql_replication_hostgroups'\n");
proxy_warning("ONLINE UPGRADE of table 'pgsql_replication_hostgroups' in progress\n");
// drop any existing table with suffix _v301
configdb->execute("DROP TABLE IF EXISTS pgsql_replication_hostgroups_v301");
// rename current table to add suffix _v301
configdb->execute("ALTER TABLE pgsql_replication_hostgroups RENAME TO pgsql_replication_hostgroups_v301");
// create new table
configdb->build_table(
const_cast<char*>("pgsql_replication_hostgroups"),
const_cast<char*>(ADMIN_SQLITE_TABLE_PGSQL_REPLICATION_HOSTGROUPS),
false
);
// copy fields from old table
configdb->execute(
"INSERT INTO pgsql_replication_hostgroups(writer_hostgroup, reader_hostgroup, check_type, comment)"
" SELECT writer_hostgroup, reader_hostgroup, 'read_only', comment FROM pgsql_replication_hostgroups_v301"
);
}

configdb->execute("PRAGMA foreign_keys = ON");
}

Choose a reason for hiding this comment

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

high

The database migration logic should be wrapped in a transaction to ensure atomicity. If any of the configdb->execute() or configdb->build_table() calls fail, the database could be left in an inconsistent state. For example, if the build_table call fails after the ALTER TABLE ... RENAME has succeeded, the pgsql_replication_hostgroups table will be missing, which could prevent ProxySQL from starting correctly. To ensure atomicity, the entire migration process within the if (rci) block should be wrapped in a database transaction. This will ensure that all changes are committed together, or rolled back if an error occurs.

configdb->execute("PRAGMA foreign_keys = OFF");

	int rci = configdb->check_table_structure(
		const_cast<char*>("pgsql_replication_hostgroups"),
		const_cast<char*>(ADMIN_SQLITE_TABLE_PGSQL_REPLICATION_HOSTGROUPS_V3_0_1)
	);

	if (rci) {
		// upgrade is required
		proxy_warning("Detected version pre-3.0.2 of table 'pgsql_replication_hostgroups'\n");
		proxy_warning("ONLINE UPGRADE of table 'pgsql_replication_hostgroups' in progress\n");
		configdb->execute("BEGIN TRANSACTION");
		// drop any existing table with suffix _v301
		configdb->execute("DROP TABLE IF EXISTS pgsql_replication_hostgroups_v301");
		// rename current table to add suffix _v301
		configdb->execute("ALTER TABLE pgsql_replication_hostgroups RENAME TO pgsql_replication_hostgroups_v301");
		// create new table
		configdb->build_table(
			const_cast<char*>("pgsql_replication_hostgroups"),
			const_cast<char*>(ADMIN_SQLITE_TABLE_PGSQL_REPLICATION_HOSTGROUPS),
			false
		);
		// copy fields from old table
		configdb->execute(
			"INSERT INTO pgsql_replication_hostgroups(writer_hostgroup, reader_hostgroup, check_type, comment)"
				" SELECT writer_hostgroup, reader_hostgroup, 'read_only', comment FROM pgsql_replication_hostgroups_v301"
		);
		configdb->execute("COMMIT");
	}

	configdb->execute("PRAGMA foreign_keys = ON");

@renecannao
Copy link
Contributor

Can one of the admins verify this patch?

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@renecannao renecannao merged commit afb1865 into v3.0 Aug 4, 2025
3 of 5 checks passed
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.

2 participants